Issue 2148 - Rewrite the testpages test suite

Issue 2148 - Rewrite the testpages test suite.

Removing old testsuite/cases. Adding test cases for filter types, exceptions and some circumvention examples.

The generic hide/block test pages still need to be recreated but I will do that in a future commit.

This push is so that the pages can be used for AA filter type testing and so that the circumvention team can begin to create pages for testing CSP filters on top of this commit.

[rossg, 2018-04-18 12:19:24]

Patch Set 1

[kzar, 2018-04-19 13:08:28]

Great work Ross, it looks way better than the old site already! I've not
reviewed everything (not easily possible with such a large patch set) but I've
made a start.

For anyone else taking a look Rietveld has manged the patch a bit, to get it to
apply I had to do the following:

  wget https://codereview.adblockplus.org/download/issue29755578_29755579.diff
  patchconv < issue29755578_29755579.diff > fixed.diff
  git apply --ignore-whitespace fixed.diff

Usability-wise I was a bit confused about what I should do to run a test, or
what was the expected output on a success or failure. For example I added the
subscription which worked fine, but then when I clicked "Blocking" I didn't know
what to do next or if it had worked.

Perhaps adding some instructions or making it more obvious would help? Maybe we
should add a couple of paragraphs after the Welcome message to explain how to
use the website?

  Welcome
  This site is designed to help test Adblock Plus. To get started...

and perhaps each test case should have similar instructions:

  Blocking
  Check that basic blocking functionality is working. Please do X and confirm Y
happens, not Z...

Also I noticed there's no favicon so far, perhaps you can just nick the one from
adblockplus.org?

https://codereview.adblockplus.org/29755578/diff/29755579/includes/footer.tmpl
File includes/footer.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/includes/footer.tmp...
includes/footer.tmpl:2: <p class="site-update">Last site update: April 2018</p>
I think it would be better to display a version number or commit hash here. That
way when people report bugs they can be specific about which version of the test
pages caused the problem. What do you think?

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/el...
File pages/exceptions/elemhide.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/el...
pages/exceptions/elemhide.tmpl:22:
testpages.adblockplus.org##.testcase-ex-elemhide<br>
Perhaps put the example filters inside pre tags and style them with a monospace
font?

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
File pages/exceptions/xmlhttprequest.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
pages/exceptions/xmlhttprequest.tmpl:9: function pagelog(message) {
This function is in the code 5 times in different places. Perhaps create a file
like common.js and put shared JavaScript code in that?

https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/image...
File pages/filters/image.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/image...
pages/filters/image.tmpl:23: <h3>Stylesheet</h3><div
class="testcase-teststylesheet"></div>
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/ping....
File pages/filters/ping.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/ping....
pages/filters/ping.tmpl:1: title = $ping - ABP Test Pages
Nit: Instead of hard-coding " - ABP Test Pages" on the end of all the titles,
why not do that in default.tmpl and the other templates? Something like this:

<title>{{ title }} - ABP Test Pages</title>

https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/ping....
pages/filters/ping.tmpl:20: pagelog( "Calling navigator.sendBeacon()" );
Nit: In JavaScript code we generally don't put whitespace directly after ( or
before ). 

In other words, instead of:

pagelog( "Calling navigator.sendBeacon()" );

please do:

pagelog("Calling navigator.sendBeacon()");

https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/ping....
pages/filters/ping.tmpl:24: (res ? pagelog("navigator.sendBeacon() returned
true.") : pagelog("Beacon could not be queued."));
Nit: Please use a regular if... else here, since you're not actually using the
return value of the pagelog calls (untested):

if (res)
  pagelog("navigator.sendBeacon() returned true.");
else
  pagelog("Beacon could not be queued.");

Alternatively you could put the inline conditional inside the pagelog call
(untested):

pagelog(res ? "navigator.sendBeacon() returned true." : "Beacon could not be
queued.");

https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/scrip...
File pages/filters/script.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/scrip...
pages/filters/script.tmpl:22: <h3>Image</h3><img class="testcase-image"
src="/testcasefiles/script/image.jpg"/>
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/webrt...
File pages/filters/webrtc.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/webrt...
pages/filters/webrtc.tmpl:26: 
Nit: Please could you remove the trailing whitespace here?

https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/webrt...
pages/filters/webrtc.tmpl:40: 
Nit: Please could you remove the trailing whitespace here?

https://codereview.adblockplus.org/29755578/diff/29755579/pages/index.md
File pages/index.md (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/index.md#newc...
pages/index.md:9: <? include testcases ?>
Nit: Please could you remove the trailing whitespace here?

https://codereview.adblockplus.org/29755578/diff/29755579/sources/object-subr...
File sources/object-subrequest.as (right):

https://codereview.adblockplus.org/29755578/diff/29755579/sources/object-subr...
sources/object-subrequest.as:18: 
Nit: Please could you remove the trailing whitespace here?

https://codereview.adblockplus.org/29755578/diff/29755579/sources/object-subr...
sources/object-subrequest.as:20: }
Nit: Please could you remove the trailing whitespace here?

https://codereview.adblockplus.org/29755578/diff/29755579/sources/object.as
File sources/object.as (right):

https://codereview.adblockplus.org/29755578/diff/29755579/sources/object.as#n...
sources/object.as:23: }
Nit: Please could you remove the trailing whitespace here?

https://codereview.adblockplus.org/29755578/diff/29755579/static/css/testpage...
File static/css/testpages.css (right):

https://codereview.adblockplus.org/29755578/diff/29755579/static/css/testpage...
static/css/testpages.css:268: max-width: 900px;
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
File static/testcasefiles/document/stylesheet.css (right):

https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
static/testcasefiles/document/stylesheet.css:2: background: none;
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
static/testcasefiles/document/stylesheet.css:9: padding: .5rem;
I noticed .testcase-scripttarget (the message on the script page) doesn't get
the same padding. Did you forget to add that class in here?

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
File templates/testcase.tmpl (left):

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:22: <? include footer ?>
Nit: Please could you remove this tab character? It's fie for this tag to start
at the beginning of the line.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
File templates/testcase.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:6: <meta name="viewport" content="width=device-width,
initial-scale=1">
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:11: 
Nit: Please could you remove the trailing whitespace here?

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:13: <a href="/" target="_top">Home</a>
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:14: > <a href="testcases" target="_top">Test cases</a>
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:16: </section>
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:39: </section>
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:40: {% endif %}
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:44: <section class="site-panel">
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:53: </section>
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:54: {% endif %}
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:56: {% if testcase_passnotes %}
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:62: </section>
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:63: {% endif %}
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:65: {% if testcase_moreinfo %}
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:66: <section class="site-panel">
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:73: </section>
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:74: {% endif %}
Nit: Git warned me that you've accidentally put a tab character (instead of
spaces) here.

https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
templates/testcase.tmpl:81: 
Nit: Please could you remove these extra blank lines? Leave one, before the
include, but be careful to strip any spaces / tabs from it.

[rossg, 2018-04-23 09:42:20]

On 2018/04/19 13:08:28, kzar wrote:
> Great work Ross, it looks way better than the old site already! I've not
> reviewed everything (not easily possible with such a large patch set) but I've
> made a start.

Thanks.

> 
> For anyone else taking a look Rietveld has manged the patch a bit, to get it
to
> apply I had to do the following:
> 
>   wget https://codereview.adblockplus.org/download/issue29755578_29755579.diff
>   patchconv < issue29755578_29755579.diff > fixed.diff
>   git apply --ignore-whitespace fixed.diff
> 
> Usability-wise I was a bit confused about what I should do to run a test, or
> what was the expected output on a success or failure. For example I added the
> subscription which worked fine, but then when I clicked "Blocking" I didn't
know
> what to do next or if it had worked.
> 
> Perhaps adding some instructions or making it more obvious would help? Maybe
we
> should add a couple of paragraphs after the Welcome message to explain how to
> use the website?
> 
>   Welcome
>   This site is designed to help test Adblock Plus. To get started...
> 
> and perhaps each test case should have similar instructions:
> 
>   Blocking
>   Check that basic blocking functionality is working. Please do X and confirm
Y
> happens, not Z...

I agree with these points. In general I was aiming for "red boxes bad,
grey/green boxes good" but agree it could still do with some explaination. I was
attempting to avoid adding too much text too soon. I would prefer to add text in
another issue/commit as then I can standadise it's presentation without it all
getting lost in the test cases themselves.

> 
> Also I noticed there's no favicon so far, perhaps you can just nick the one
from
> adblockplus.org?

Could do.

> 
> https://codereview.adblockplus.org/29755578/diff/29755579/includes/footer.tmpl
> File includes/footer.tmpl (right):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/includes/footer.tmp...
> includes/footer.tmpl:2: <p class="site-update">Last site update: April
2018</p>
> I think it would be better to display a version number or commit hash here.
That
> way when people report bugs they can be specific about which version of the
test
> pages caused the problem. What do you think?

That would be fine, however wouldn't that require some sort of build script or
hook to insert the revision/commit hash into the file? I'm trying to avoid
overcomplicating the testpages for now for the most part.


>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/el...
> File pages/exceptions/elemhide.tmpl (right):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/el...
> pages/exceptions/elemhide.tmpl:22:
> testpages.adblockplus.org##.testcase-ex-elemhide<br>
> Perhaps put the example filters inside pre tags and style them with a
monospace
> font?

Yep, I plan to do this with pre, monospace, and overflow-scroll: x so they are
easy to copy and don't line break (as they currently do on mobile). Again I'd
like to do this as part of a seperate issue unless you think it should be done
as part of this one.

> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
> File pages/exceptions/xmlhttprequest.tmpl (right):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
> pages/exceptions/xmlhttprequest.tmpl:9: function pagelog(message) {
> This function is in the code 5 times in different places. Perhaps create a
file
> like common.js and put shared JavaScript code in that?

I did think about that but decided not to introduce another request for just one
function. Currently each test page is currently the page, the global css, then
any requests/resources required for the test case itself, which makes sense to
me?

> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/image...
> File pages/filters/image.tmpl (right):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/image...
> pages/filters/image.tmpl:23: <h3>Stylesheet</h3><div
> class="testcase-teststylesheet"></div>
> Nit: Git warned me that you've accidentally put a tab character (instead of
> spaces) here.

Will fix.

> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/ping....
> File pages/filters/ping.tmpl (right):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/ping....
> pages/filters/ping.tmpl:1: title = $ping - ABP Test Pages
> Nit: Instead of hard-coding " - ABP Test Pages" on the end of all the titles,
> why not do that in default.tmpl and the other templates? Something like this:
> 
> <title>{{ title }} - ABP Test Pages</title>

I agree. Will fix.

> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/ping....
> pages/filters/ping.tmpl:20: pagelog( "Calling navigator.sendBeacon()" );
> Nit: In JavaScript code we generally don't put whitespace directly after ( or
> before ). 
> 
> In other words, instead of:
> 
> pagelog( "Calling navigator.sendBeacon()" );
> 
> please do:
> 
> pagelog("Calling navigator.sendBeacon()");

Will fix :)

> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/ping....
> pages/filters/ping.tmpl:24: (res ? pagelog("navigator.sendBeacon() returned
> true.") : pagelog("Beacon could not be queued."));
> Nit: Please use a regular if... else here, since you're not actually using the
> return value of the pagelog calls (untested):
> 
> if (res)
>   pagelog("navigator.sendBeacon() returned true.");
> else
>   pagelog("Beacon could not be queued.");
> 
> Alternatively you could put the inline conditional inside the pagelog call
> (untested):
> 
> pagelog(res ? "navigator.sendBeacon() returned true." : "Beacon could not be
> queued.");

Ah, I had a normal if/else here originally. Will fix :)

> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/scrip...
> File pages/filters/script.tmpl (right):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/scrip...
> pages/filters/script.tmpl:22: <h3>Image</h3><img class="testcase-image"
> src="/testcasefiles/script/image.jpg"/>
> Nit: Git warned me that you've accidentally put a tab character (instead of
> spaces) here.

Will fix.

> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/webrt...
> File pages/filters/webrtc.tmpl (right):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/webrt...
> pages/filters/webrtc.tmpl:26: 
> Nit: Please could you remove the trailing whitespace here?

Will fix.

> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/filters/webrt...
> pages/filters/webrtc.tmpl:40: 
> Nit: Please could you remove the trailing whitespace here?

Will fix.

> 
> https://codereview.adblockplus.org/29755578/diff/29755579/pages/index.md
> File pages/index.md (right):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/pages/index.md#newc...
> pages/index.md:9: <? include testcases ?>
> Nit: Please could you remove the trailing whitespace here?
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/sources/object-subr...
> File sources/object-subrequest.as (right):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/sources/object-subr...
> sources/object-subrequest.as:18: 
> Nit: Please could you remove the trailing whitespace here?
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/sources/object-subr...
> sources/object-subrequest.as:20: }
> Nit: Please could you remove the trailing whitespace here?
> 
> https://codereview.adblockplus.org/29755578/diff/29755579/sources/object.as
> File sources/object.as (right):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/sources/object.as#n...
> sources/object.as:23: }
> Nit: Please could you remove the trailing whitespace here?
> 
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
> File static/testcasefiles/document/stylesheet.css (right):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
> static/testcasefiles/document/stylesheet.css:2: background: none;
> Nit: Git warned me that you've accidentally put a tab character (instead of
> spaces) here.

Will fix above whitespace issues.

> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
> static/testcasefiles/document/stylesheet.css:9: padding: .5rem;
> I noticed .testcase-scripttarget (the message on the script page) doesn't get
> the same padding. Did you forget to add that class in here?

Yep it just needs padding. Will fix.

> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
> File templates/testcase.tmpl (left):
> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
> templates/testcase.tmpl:22: <? include footer ?>
> Nit: Please could you remove this tab character? It's fie for this tag to
start
> at the beginning of the line.

Will fix.

> Snipped quote for 10000 char limit.

Will fix above whitespace issues.

> 
>
https://codereview.adblockplus.org/29755578/diff/29755579/templates/testcase....
> templates/testcase.tmpl:81: 
> Nit: Please could you remove these extra blank lines? Leave one, before the
> include, but be careful to strip any spaces / tabs from it.

Will fix.

[kzar, 2018-04-23 09:57:28]

Please respond to inline code comments by clicking Reply by the comment itself.
You go through all the inline comments writing draft responses, then you click
"Publish + Mail comments" and add anything general you want to say in the
message box there. They all get sent out together, and the individual
conversations stay next to the code they're discussing. The reply button at the
bottom of the page should only be used if you're not responding to any inline
comments.

> I agree with these points. In general I was aiming for "red boxes
> bad, grey/green boxes good" but agree it could still do with some
> explaination. I was attempting to avoid adding too much text too
> soon. I would prefer to add text in another issue/commit as then I
> can standardise it's presentation without it all getting lost in the
> test cases themselves.

It's fair to want to avoid too much text, but at the moment it's not
clear how to use the tests which IMO is a bigger problem.

https://codereview.adblockplus.org/29755578/diff/29755579/includes/footer.tmpl
File includes/footer.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/includes/footer.tmp...
includes/footer.tmpl:2: <p class="site-update">Last site update: April 2018</p>
> > I think it would be better to display a version number or commit hash 
> > here. That way when people report bugs they can be specific about which 
> > version of the test pages caused the problem. What do you think?
>
> That would be fine, however wouldn't that require some sort of build
> script or hook to insert the revision/commit hash into the file? I'm
> trying to avoid overcomplicating the testpages for now for the most
> part.

Yea, fair enough.

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/el...
File pages/exceptions/elemhide.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/el...
pages/exceptions/elemhide.tmpl:22:
testpages.adblockplus.org##.testcase-ex-elemhide<br>
On 2018/04/19 13:08:20, kzar wrote:
> > Perhaps put the example filters inside pre tags and style them
> > with a monospace font?
>
> Yep, I plan to do this with pre, monospace, and overflow-scroll: x so they are
> easy to copy and don't line break (as they currently do on mobile). Again I'd
> like to do this as part of a seperate issue unless you think it should be done
> as part of this one.

Well I think you should do it now personally, you already redesigned everything
with this change anyway.

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
File pages/exceptions/xmlhttprequest.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
pages/exceptions/xmlhttprequest.tmpl:9: function pagelog(message) {
> > This function is in the code 5 times in different places. Perhaps
> > create a file like common.js and put shared JavaScript code in that?
>
> I did think about that but decided not to introduce another request
> for just one function. Currently each test page is currently the
> page, the global css, then any requests/resources required for
> the test case itself, which makes sense to me?

I think the code duplication is a bigger problem than an extra request. I mean
if you need to tweak that function it's already going to be a pain and this
is going to get worse as we add further test cases.

If you think the extra request matters (I don't personally) you could put the
common JavaScript code inline in the shared template.

[kzar, 2018-04-23 10:26:42]

> Will fix.

BTW there's generally no need for these "Will fix" comments. Just respond
"Done." at the same time as you're submitting a patch set that addresses the
comment. (Or maybe some other message if you've done something else or
disagree.)

The one time I think these comments are useful are when you're submitting a
patch set which addresses most of the feedback, but there's one or two which you
haven't had time to address. Mentioning something like "I haven't forgotten
about this one" can be useful to reassure the reviewer.

[juliandoucette, 2018-04-23 14:12:30]

Do you want me to review something specific (or the entire website contents of
this change)?

[kzar, 2018-04-23 14:14:51]

On 2018/04/23 14:12:30, juliandoucette wrote:
> Do you want me to review something specific (or the entire website contents of
> this change)?

If you have any feedback about the CSS and or HTML markup that would be useful,
even if you don't look through everything.

[rossg, 2018-04-25 10:26:34]

On 2018/04/23 10:26:42, kzar wrote:
> > Will fix.
> 
> BTW there's generally no need for these "Will fix" comments. Just respond
> "Done." at the same time as you're submitting a patch set that addresses the
> comment. (Or maybe some other message if you've done something else or
> disagree.)
> 
> The one time I think these comments are useful are when you're submitting a
> patch set which addresses most of the feedback, but there's one or two which
you
> haven't had time to address. Mentioning something like "I haven't forgotten
> about this one" can be useful to reassure the reviewer.

Thank you for the explationation. I hope to address everything this afternoon.

[rossg, 2018-06-04 13:24:41]

Addressed Dave's feedback.

Added descriptions/help text. Added favicon. Added pre tags. Fixed whitespace
issues.

[kzar, 2018-06-04 17:17:30]

> It's fair to want to avoid too much text, but at the moment it's not
> clear how to use the tests which IMO is a bigger problem.

Thanks I found the explanations really useful. Quickly knowing what was
supposed to happen really helped me get this running locally.

It would be cool if the URL "https://testpages.adblockplus.org" wasn't
hard-coded everywhere, or if it could be configured in settings.ini at
least. To get this working I had to point testpages.adblockplus.org at
localhost and also use port 80 instead of the default 5000.

But if that's not practical for now perhaps we should add a note about
that to the README. That way people trying to run the project locally
will be given a heads-up. I have to admit it took me a minute to realise
why the subscription link didn't work, and latest why most of the test
cases failed since I was using http://localhost:5000 instead.

Anyway it's looking way better now, mostly just a few nits left.

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
File pages/exceptions/xmlhttprequest.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
pages/exceptions/xmlhttprequest.tmpl:9: function pagelog(message) {
On 2018/04/23 09:57:27, kzar wrote:
> This function is in the code 5 times in different places. Perhaps
> create a file like common.js and put shared JavaScript code in that?
> ...
> If you think the extra request matters (I don't personally) you could 
> put the common JavaScript code inline in the shared template.

I still think it would be better if we avoided duplicating this code.

https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
File static/testcasefiles/document/stylesheet.css (right):

https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
static/testcasefiles/document/stylesheet.css:9: padding: .5rem;
On 2018/04/19 13:08:23, kzar wrote:
> I noticed .testcase-scripttarget (the message on the script page) doesn't get
> the same padding. Did you forget to add that class in here?

What about this comment?

https://codereview.adblockplus.org/29755578/diff/29798591/includes/testcases.md
File includes/testcases.md (right):

https://codereview.adblockplus.org/29755578/diff/29798591/includes/testcases....
includes/testcases.md:4: <p>You can add this subscription and then click through
to each test case page and check that the page state matches the test case
description. Alternatively you could add a test cases filter(s) manually in
order to check it in isolation.</p>
Nit: Please can you delete the trailing space?

https://codereview.adblockplus.org/29755578/diff/29798591/pages/circumvention...
File pages/circumvention/anoniframe-documentwrite.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29798591/pages/circumvention...
pages/circumvention/anoniframe-documentwrite.tmpl:1: title = Anonymous Frame
document.write() - Circumvention
Nit: Please can you delete the trailing space?

https://codereview.adblockplus.org/29755578/diff/29798591/pages/filters/block...
File pages/filters/blocking.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29798591/pages/filters/block...
pages/filters/blocking.tmpl:8: <section class="site-panel">
I wonder if we should improve the markup a bit so it's easier to style going
forward? For example perhaps we should give the title selection another class
something like "testcase-title". Julian might be a better person to weigh in on
this one though.

https://codereview.adblockplus.org/29755578/diff/29798591/pages/filters/ping....
File pages/filters/ping.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29798591/pages/filters/ping....
pages/filters/ping.tmpl:24: if(res) {
Nit: This looks better, but still a couple of minor things. Please could you add
a space after "if"? Also no need for the braces since both clauses are only one
line. Also please could you add semi colons on the end of the pagelog calls?

[rossg, 2018-06-07 10:30:26]

> It would be cool if the URL "https://testpages.adblockplus.org" wasn't
hard-coded everywhere ...

I completely agree and have added a readme file for now. I will make a new
ticket for doing this.

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
File pages/exceptions/xmlhttprequest.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
pages/exceptions/xmlhttprequest.tmpl:9: function pagelog(message) {
On 2018/06/04 17:17:28, kzar wrote:
> On 2018/04/23 09:57:27, kzar wrote:
> > This function is in the code 5 times in different places. Perhaps
> > create a file like common.js and put shared JavaScript code in that?
> > ...
> > If you think the extra request matters (I don't personally) you could 
> > put the common JavaScript code inline in the shared template.
> 
> I still think it would be better if we avoided duplicating this code.

I have inlined the function into the testcases template for now and updated the
test pages that use it.

https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
File static/testcasefiles/document/stylesheet.css (right):

https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
static/testcasefiles/document/stylesheet.css:9: padding: .5rem;
On 2018/06/04 17:17:28, kzar wrote:
> On 2018/04/19 13:08:23, kzar wrote:
> > I noticed .testcase-scripttarget (the message on the script page) doesn't
get
> > the same padding. Did you forget to add that class in here?
> 
> What about this comment?

I had addressed that by adding padding to .testcase-scripttarget globally in the
sites stylesheet.

https://codereview.adblockplus.org/29755578/diff/29798591/includes/testcases.md
File includes/testcases.md (right):

https://codereview.adblockplus.org/29755578/diff/29798591/includes/testcases....
includes/testcases.md:4: <p>You can add this subscription and then click through
to each test case page and check that the page state matches the test case
description. Alternatively you could add a test cases filter(s) manually in
order to check it in isolation.</p>
On 2018/06/04 17:17:28, kzar wrote:
> Nit: Please can you delete the trailing space?

Done.

https://codereview.adblockplus.org/29755578/diff/29798591/pages/circumvention...
File pages/circumvention/anoniframe-documentwrite.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29798591/pages/circumvention...
pages/circumvention/anoniframe-documentwrite.tmpl:1: title = Anonymous Frame
document.write() - Circumvention
On 2018/06/04 17:17:29, kzar wrote:
> Nit: Please can you delete the trailing space?

Done.

https://codereview.adblockplus.org/29755578/diff/29798591/pages/filters/block...
File pages/filters/blocking.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29798591/pages/filters/block...
pages/filters/blocking.tmpl:8: <section class="site-panel">
On 2018/06/04 17:17:29, kzar wrote:
> I wonder if we should improve the markup a bit so it's easier to style going
> forward? For example perhaps we should give the title selection another class
> something like "testcase-title". Julian might be a better person to weigh in
on
> this one though.

I plan to do this somewhat with the next wave of test pages if thats okay?

https://codereview.adblockplus.org/29755578/diff/29798591/pages/filters/ping....
File pages/filters/ping.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29798591/pages/filters/ping....
pages/filters/ping.tmpl:24: if(res) {
On 2018/06/04 17:17:29, kzar wrote:
> Nit: This looks better, but still a couple of minor things. Please could you
add
> a space after "if"? Also no need for the braces since both clauses are only
one
> line. Also please could you add semi colons on the end of the pagelog calls?

Done.

[kzar, 2018-06-07 11:43:06]

One tiny nit in the README but otherwise LGTM. Nice work.

> I completely agree and have added a readme file for now. I 
> will make a new ticket for doing this.

Ace, thanks.

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
File pages/exceptions/xmlhttprequest.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29755579/pages/exceptions/xm...
pages/exceptions/xmlhttprequest.tmpl:9: function pagelog(message) {
On 2018/06/07 10:30:23, rossg wrote:
> On 2018/06/04 17:17:28, kzar wrote:
> > On 2018/04/23 09:57:27, kzar wrote:
> > > This function is in the code 5 times in different places. Perhaps
> > > create a file like common.js and put shared JavaScript code in that?
> > > ...
> > > If you think the extra request matters (I don't personally) you could 
> > > put the common JavaScript code inline in the shared template.
> > 
> > I still think it would be better if we avoided duplicating this code.
> 
> I have inlined the function into the testcases template for now and updated
the
> test pages that use it.

Great, thanks.

https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
File static/testcasefiles/document/stylesheet.css (right):

https://codereview.adblockplus.org/29755578/diff/29755579/static/testcasefile...
static/testcasefiles/document/stylesheet.css:9: padding: .5rem;
On 2018/06/07 10:30:23, rossg wrote:
> On 2018/06/04 17:17:28, kzar wrote:
> > On 2018/04/19 13:08:23, kzar wrote:
> > > I noticed .testcase-scripttarget (the message on the script page) doesn't
> get
> > > the same padding. Did you forget to add that class in here?
> > 
> > What about this comment?
> 
> I had addressed that by adding padding to .testcase-scripttarget globally in
the
> sites stylesheet.

Acknowledged.

https://codereview.adblockplus.org/29755578/diff/29798591/pages/filters/block...
File pages/filters/blocking.tmpl (right):

https://codereview.adblockplus.org/29755578/diff/29798591/pages/filters/block...
pages/filters/blocking.tmpl:8: <section class="site-panel">
On 2018/06/07 10:30:24, rossg wrote:
> On 2018/06/04 17:17:29, kzar wrote:
> > I wonder if we should improve the markup a bit so it's easier to style going
> > forward? For example perhaps we should give the title selection another
class
> > something like "testcase-title". Julian might be a better person to weigh in
> on
> > this one though.
> 
> I plan to do this somewhat with the next wave of test pages if thats okay?

Sure, if you want to do this later that's fair enough. I'd prefer it if you did
that separately, instead of along with the next wave of test pages though if
possible. Huge changes are quite hard to review.

https://codereview.adblockplus.org/29755578/diff/29801569/README.md
File README.md (right):

https://codereview.adblockplus.org/29755578/diff/29801569/README.md#newcode1
README.md:1: # testpages.adblockplus web content
Nit: This is missing the ".org"

[rossg, 2018-06-07 13:59:43]

Fixed README title.

Noted your comment about fixing hard-coded urls seperate to more pages.

https://codereview.adblockplus.org/29755578/diff/29801569/README.md
File README.md (right):

https://codereview.adblockplus.org/29755578/diff/29801569/README.md#newcode1
README.md:1: # testpages.adblockplus web content
On 2018/06/07 11:43:05, kzar wrote:
> Nit: This is missing the ".org"

Done.

[kzar, 2018-06-07 14:22:29]

LGTM

[juliandoucette, 2018-06-08 17:10:23]

I haven't read up on what this is or how it works yet.

@Manvel do you already know and want to look this over? I think the current
implementation has already been pushed and is causing a build error (unless that
was fixed already).

[saroyanm, 2018-06-11 07:34:57]

On 2018/06/08 17:10:23, juliandoucette wrote:
> I haven't read up on what this is or how it works yet.
> 
> @Manvel do you already know and want to look this over? I think the current
> implementation has already been pushed and is causing a build error (unless
that
> was fixed already).
I haven't worked with the test pages much either, not sure what you want me look
at exactly ?
Also if this pushed already I think it's better to file a separate issue to fix
the build errors.

Sidenote: In general if Websites team doesn't maintain this code, maybe we can
make a note in the modules file, but probably it's a separate discussion.