Issue 6769 - Avoid hardcoding the URL

Issue 6769 - Avoid hardcoding the URL

[kzar, 2018-09-04 16:25:12]

Patch Set 1

I've not carefully checked through and tested these changes yet, I'll have
another look through them again tomorrow.

[Sebastian Noack, 2018-09-04 16:45:56]

Just in case, did you try running "npm test" from the "next" branch in
"adblockpluschrome" against the test pages with these changes?

I guess, the simplest way to do so would be by patching
adblockpluschrome/test/wrappers/pages.js, to use http://localhost:5000/ as entry
point.

[kzar, 2018-09-05 17:02:52]

Patch Set 2 : Fixed some mistakes

> I've not carefully checked through and tested these 
> changes yet, I'll have another look through them again
> tomorrow.

OK, I've done that now.

> I guess, the simplest way to do so would be by patching
> adblockpluschrome/test/wrappers/pages.js, to use
> http://localhost:5000/ as entry point.

Cool! I guess that's all new since I've been away! It passes now, but it did
require a small tweak to the core code for the localhost domain - see
https://issues.adblockplus.org/ticket/6931 .

https://codereview.adblockplus.org/29874607/diff/29874608/templates/plaintext...
File templates/plaintext.tmpl (right):

https://codereview.adblockplus.org/29874607/diff/29874608/templates/plaintext...
templates/plaintext.tmpl:1: {{ body|trim|safe }}
The `trim` was necessary since there was a blank line showing up at the start of
the filters file otherwise, which upset the extension. Perhaps there's a better
way to avoid that, but I couldn't figure it out. I'm open to suggestions anyway.

[Sebastian Noack, 2018-09-19 11:28:39]

Would you mind rebasing these changes?

[kzar, 2018-09-24 13:40:20]

Patch Set 3: Rebased

OK, there you go, I've put the line endings of the filter list file back to DOS
too.

By the way, the change is also in GitHub[1] if you're still having trouble with
the patch Rietveld provides.

[1] - https://github.com/kzar/testpages.adblockplus.org/tree/6769-url-hardcode

[Sebastian Noack, 2018-09-24 15:56:29]

Thanks, I could download and apply the patch from GitHub.

But with the test automation I get following test failure:

AssertionError [ERR_ASSERTION]: subscribe link: subscription added

[kzar, 2018-09-25 11:15:53]

On 2018/09/24 15:56:29, Sebastian Noack wrote:
> Thanks, I could download and apply the patch from GitHub.
> 
> But with the test automation I get following test failure:
> 
> AssertionError [ERR_ASSERTION]: subscribe link: subscription added

I can't reproduce that, which revision of adlbockpluschrome are you testing
against? (I'm testing against next so
https://github.com/adblockplus/adblockpluschrome/commit/ba3d23762a49daefab07a...)

[Sebastian Noack, 2018-09-25 12:02:52]

On 2018/09/25 11:15:53, kzar wrote:
> On 2018/09/24 15:56:29, Sebastian Noack wrote:
> > Thanks, I could download and apply the patch from GitHub.
> > 
> > But with the test automation I get following test failure:
> > 
> > AssertionError [ERR_ASSERTION]: subscribe link: subscription added
> 
> I can't reproduce that, which revision of adlbockpluschrome are you testing
> against? (I'm testing against next so
>
https://github.com/adblockplus/adblockpluschrome/commit/ba3d23762a49daefab07a...)

Current master (hg: 0db859ce10b2). Note that there is no "next" at the moment.
It seems that the synchronization to GitHub doesn't remove branches that no
longer exist in Mercurial. So the "next" branch there is outdated.

[kzar, 2018-09-25 13:12:04]

On 2018/09/25 12:02:52, Sebastian Noack wrote:
> On 2018/09/25 11:15:53, kzar wrote:
> > On 2018/09/24 15:56:29, Sebastian Noack wrote:
> > > Thanks, I could download and apply the patch from GitHub.
> > > 
> > > But with the test automation I get following test failure:
> > > 
> > > AssertionError [ERR_ASSERTION]: subscribe link: subscription added
> > 
> > I can't reproduce that, which revision of adlbockpluschrome are you testing
> > against? (I'm testing against next so
> >
>
https://github.com/adblockplus/adblockpluschrome/commit/ba3d23762a49daefab07a...)
> 
> Current master (hg: 0db859ce10b2). Note that there is no "next" at the moment.
> It seems that the synchronization to GitHub doesn't remove branches that no
> longer exist in Mercurial. So the "next" branch there is outdated.

All the tests pass for me with 0db859ce10b2 and the TEST_PAGES_URL of
"http://localhost:5000/en/" intermittently. I suspect there are some problems
with the test automation that are causing the occasional fails. Does it fail for
your consistently? Does it work consistently without the TEST_PAGES_URL change?

[Sebastian Noack, 2018-09-25 13:56:05]

On 2018/09/25 13:12:04, kzar wrote:
> All the tests pass for me with 0db859ce10b2 and the TEST_PAGES_URL of
> "http://localhost:5000/en/" intermittently. I suspect there are some problems
> with the test automation that are causing the occasional fails. Does it fail
for
> your consistently? Does it work consistently without the TEST_PAGES_URL
change?

The problem was that I set TEST_PAGES_URL to "http://localhost:5000/". If I set
it to "http://localhost:5000/en/" the tests pass. LGTM.