Issue 29328998: Issue 132 - Add REAMDE to adblockplustests repository (Closed)
DescriptionIssue 132 - Add REAMDE to adblockplustests repository
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed feedback #MessagesTotal messages: 7
Patch Set 1
https://codereview.adblockplus.org/29328998/diff/29328999/README.md File README.md (right): https://codereview.adblockplus.org/29328998/diff/29328999/README.md#newcode55 README.md:55: ### Chrome IMO this section is irrelevant and redundant here.
https://codereview.adblockplus.org/29328998/diff/29328999/README.md File README.md (right): https://codereview.adblockplus.org/29328998/diff/29328999/README.md#newcode55 README.md:55: ### Chrome On 2015/10/09 12:00:09, Sebastian Noack wrote: > IMO this section is irrelevant and redundant here. Yea, maybe you're right. I wasn't sure either way on this one. My reason for wanting to include _something_ about it was that I did not realise that Adblock Plus for Chrome used any of these tests initially. I found it a little surprising. (I'm pretty sure I pushed changed to this repository without first checking the Chrome tests still passed before I realised this.)
https://codereview.adblockplus.org/29328998/diff/29328999/README.md File README.md (right): https://codereview.adblockplus.org/29328998/diff/29328999/README.md#newcode6 README.md:6: Firefox extension `adblockplus`. This test suite is itself a separate Firefox "for the Adblock Plus Firefox extension `adblockplus`" => "for the Adblock Plus extension in Firefox" https://codereview.adblockplus.org/29328998/diff/29328999/README.md#newcode16 README.md:16: explicitly.) Please don't document technical details, these can change. I'd replace this paragraph by the following: Some of the tests in this test suite are also reused by the Chrome/Opera/Safari version of Adblock Plus. Please see the `adblockpluschrome` repository for details on how to run the tests there. https://codereview.adblockplus.org/29328998/diff/29328999/README.md#newcode52 README.md:52: - Several `Content policy: HTML embed (Java)` tests are currently failing. I'd not document specific test failures, these can get out of date very quickly. Instead: - Some tests are currently known to fail (sometimes depending on your browser version and configuration). It is better to test this before making any changes. https://codereview.adblockplus.org/29328998/diff/29328999/README.md#newcode55 README.md:55: ### Chrome I agree with Sebastian, this is unnecessary. The note above and the reference to adblockpluschrome repository are sufficient.
Patch Set 2 : Addressed feedback https://codereview.adblockplus.org/29328998/diff/29328999/README.md File README.md (right): https://codereview.adblockplus.org/29328998/diff/29328999/README.md#newcode6 README.md:6: Firefox extension `adblockplus`. This test suite is itself a separate Firefox On 2015/10/09 17:35:01, Wladimir Palant wrote: > "for the Adblock Plus Firefox extension `adblockplus`" => "for the Adblock Plus > extension in Firefox" Done. https://codereview.adblockplus.org/29328998/diff/29328999/README.md#newcode16 README.md:16: explicitly.) On 2015/10/09 17:35:01, Wladimir Palant wrote: > Please don't document technical details, these can change. I'd replace this > paragraph by the following: > > Some of the tests in this test suite are also reused by the Chrome/Opera/Safari > version of Adblock Plus. Please see the `adblockpluschrome` repository for > details on how to run the tests there. Done. https://codereview.adblockplus.org/29328998/diff/29328999/README.md#newcode52 README.md:52: - Several `Content policy: HTML embed (Java)` tests are currently failing. On 2015/10/09 17:35:01, Wladimir Palant wrote: > I'd not document specific test failures, these can get out of date very quickly. > Instead: > > - Some tests are currently known to fail (sometimes depending on your browser > version and configuration). It is better to test this before making any changes. Done. https://codereview.adblockplus.org/29328998/diff/29328999/README.md#newcode55 README.md:55: ### Chrome On 2015/10/09 17:35:01, Wladimir Palant wrote: > I agree with Sebastian, this is unnecessary. The note above and the reference to > adblockpluschrome repository are sufficient. Done.
LGTM
LGTM 
|
