Issue 4045 - Add Test Suite To CMS

The opening of the code review for a pytest based test suite for the CMS

Repository: hg.adblockplus.org/cms/

[Jon Sonesen, 2016-06-01 02:19:12]

On 2016/06/01 02:11:02, Jon Sonesen wrote:

I am have a lot more to add to this but have been trying to add a method to
generate the static files
to be compared, however it hasn't been easy since that requires a repository
that is nested in the 
cms repo which I had some problems with. Additionally, when running the generate
command on the test_site
dir with a repo in it I get an error saying there is no settings.ini when there
is so it is probably something
dumb I am doing.

[Jon Sonesen, 2016-06-01 02:36:51]

The answer to do the static comparisons may be a subrepo,
but it is considered a feature of last resort:
https://www.mercurial-scm.org/wiki/FeaturesOfLastResort
and should be avoided, any ideas about how to handle this are appreciated. I was
thinking that 
maybe since the content of that subrepo should not change it isn't too bad. But
that is just one way of looking at it
I am happy to talk about it.

[Jon Sonesen, 2016-06-02 01:20:02]

On 2016/06/01 02:36:51, Jon Sonesen wrote:
> The answer to do the static comparisons may be a subrepo,
> but it is considered a feature of last resort:
> https://www.mercurial-scm.org/wiki/FeaturesOfLastResort
> and should be avoided, any ideas about how to handle this are appreciated. I
was
> thinking that 
> maybe since the content of that subrepo should not change it isn't too bad.
But
> that is just one way of looking at it
> I am happy to talk about it.

Oops, realized that I had not properly added Vasily and Sebastian to the ticket

[Jon Sonesen, 2016-06-02 01:21:03]

On 2016/06/01 02:36:51, Jon Sonesen wrote:
> The answer to do the static comparisons may be a subrepo,
> but it is considered a feature of last resort:
> https://www.mercurial-scm.org/wiki/FeaturesOfLastResort
> and should be avoided, any ideas about how to handle this are appreciated. I
was
> thinking that 
> maybe since the content of that subrepo should not change it isn't too bad.
But
> that is just one way of looking at it
> I am happy to talk about it.

Additionally, we could just have the tet site live in a different repo that a
path can be provided for with the test command

[Vasily Kuznetsov, 2016-06-02 11:44:44]

On 2016/06/02 01:21:03, Jon Sonesen wrote:
> On 2016/06/01 02:36:51, Jon Sonesen wrote:
> > The answer to do the static comparisons may be a subrepo,
> > but it is considered a feature of last resort:
> > https://www.mercurial-scm.org/wiki/FeaturesOfLastResort
> > and should be avoided, any ideas about how to handle this are appreciated. I
> was
> > thinking that 
> > maybe since the content of that subrepo should not change it isn't too bad.
> But
> > that is just one way of looking at it
> > I am happy to talk about it.
> 
> Additionally, we could just have the tet site live in a different repo that a
> path can be provided for with the test command

I'm not sure why you want to have the test site in a different repo. It's part
of the test suite of the CMS, so it seems logical to put it in the same repo. Or
is there some other reason that I don't see?

I kind of this it like this:

The test site would be a bunch of small pages that test particular features of
the CMS. Then you can have a pytest fixture that produces different page names
for each test (see here:
https://pytest.org/latest/fixture.html#parametrizing-a-fixture) and two
dependent fixtures, one that runs that page through CMS and produces the output
and another that reads the "correct" output from a file. And the test would be
something like:

def test_static_rendering(rendered_page, approved_page):
    assert rendered_page == approved_page

Perhaps we can ignore whitespace for comparison, but that's details. Later you
could add `test_dynamic_rendering`, which would use a running CMS server and
fetch the page via web, but don't worry about it for now.

Then to get it running you need some way to produce the approved rendered pages.
You can just run the CMS over the test pages for that or (this might be better
in the long term, but don't worry if it seems like too many new things at the
same time) use something like this framework:
https://github.com/approvals/ApprovalTests.Python

How does this sound?

[Sebastian Noack, 2016-06-02 12:02:44]

On 2016/06/02 11:44:44, Vasily Kuznetsov wrote:
> I'm not sure why you want to have the test site in a different repo. It's part
> of the test suite of the CMS, so it seems logical to put it in the same repo.
Or
> is there some other reason that I don't see?

When running the cms in page generation mode, it requires the website directory
being a mercurial repo. But I'd agree, rather than putting another repo up
somewhere, it would be preferable to have the test data inncluded in the cms
repository itself. I guess we can have the test code create a temporary
mercurial repository.

> How does this sound?

Keep in mind that all features needs to be tested both, with static page
generation and with the development server. Otherwise it sounds good to me.
However, I wouldn't insist on using pytest, as it seems a custom test runner
wouldn't be any more complicated. But fine with me either way.

[Vasily Kuznetsov, 2016-06-02 12:41:15]

On 2016/06/02 12:02:44, Sebastian Noack wrote:
> On 2016/06/02 11:44:44, Vasily Kuznetsov wrote:
> > I'm not sure why you want to have the test site in a different repo. It's
part
> > of the test suite of the CMS, so it seems logical to put it in the same
repo.
> Or
> > is there some other reason that I don't see?
> 
> When running the cms in page generation mode, it requires the website
directory
> being a mercurial repo. But I'd agree, rather than putting another repo up
> somewhere, it would be preferable to have the test data inncluded in the cms
> repository itself. I guess we can have the test code create a temporary
> mercurial repository.

Ah, I didn't realise that. Yeah, just create the temporary repo and put all the
test pages into it.

> 
> > How does this sound?
> 
> Keep in mind that all features needs to be tested both, with static page
> generation and with the development server. Otherwise it sounds good to me.
> However, I wouldn't insist on using pytest, as it seems a custom test runner
> wouldn't be any more complicated. But fine with me either way.

Yeah, I don't really insist on pytest. I wouldn't bother writing a custom runner
when there's a pretty good one that can be used, but that's certainly not the
only way.

As for testing using development server, I'd first get the
static-generation-based testing up and running and then add the devserver tests
in a separate commit to keep things more small and manageable.

[Vasily Kuznetsov, 2016-06-02 12:52:07]

> Ah, I didn't realise that. Yeah, just create the temporary repo and put all
the
> test pages into it.

Also, if you decide to use pytest, this temporary repo, and probably also the
rendered site (since static generation doesn't really work page by page), could
be session-scoped fixtures (see
https://pytest.org/latest/fixture.html#sharing-a-fixture-across-tests-in-a-mo...)
to not recreate them several times.

[Jon Sonesen, 2016-06-02 17:47:35]

On 2016/06/02 12:52:07, Vasily Kuznetsov wrote:
> > Ah, I didn't realise that. Yeah, just create the temporary repo and put all
> the
> > test pages into it.
> 
> Also, if you decide to use pytest, this temporary repo, and probably also the
> rendered site (since static generation doesn't really work page by page),
could
> be session-scoped fixtures (see
>
https://pytest.org/latest/fixture.html#sharing-a-fixture-across-tests-in-a-mo...)
> to not recreate them several times.

Hey guys, thanks for talking this over with each other and myself. I can totally
do a
temporary repo that gets cleaned up after the tests are ran, I hadn't thought of
that
solution. I agree that all tests need to be ran on the test server and static
page generation.
I appreciate the feed back and will continue to post updates to this issue :D

[Jon Sonesen, 2016-06-02 17:47:38]

On 2016/06/02 12:52:07, Vasily Kuznetsov wrote:
> > Ah, I didn't realise that. Yeah, just create the temporary repo and put all
> the
> > test pages into it.
> 
> Also, if you decide to use pytest, this temporary repo, and probably also the
> rendered site (since static generation doesn't really work page by page),
could
> be session-scoped fixtures (see
>
https://pytest.org/latest/fixture.html#sharing-a-fixture-across-tests-in-a-mo...)
> to not recreate them several times.

Hey guys, thanks for talking this over with each other and myself. I can totally
do a
temporary repo that gets cleaned up after the tests are ran, I hadn't thought of
that
solution. I agree that all tests need to be ran on the test server and static
page generation.
I appreciate the feed back and will continue to post updates to this issue :D

[Jon Sonesen, 2016-06-02 18:13:54]

On 2016/06/02 11:44:44, Vasily Kuznetsov wrote:
> On 2016/06/02 01:21:03, Jon Sonesen wrote:
> > On 2016/06/01 02:36:51, Jon Sonesen wrote:
> > > The answer to do the static comparisons may be a subrepo,
> > > but it is considered a feature of last resort:
> > > https://www.mercurial-scm.org/wiki/FeaturesOfLastResort
> > > and should be avoided, any ideas about how to handle this are appreciated.
I
> > was
> > > thinking that 
> > > maybe since the content of that subrepo should not change it isn't too
bad.
> > But
> > > that is just one way of looking at it
> > > I am happy to talk about it.
> > 
> > Additionally, we could just have the tet site live in a different repo that
a
> > path can be provided for with the test command
> 
> I'm not sure why you want to have the test site in a different repo. It's part
> of the test suite of the CMS, so it seems logical to put it in the same repo.
Or
> is there some other reason that I don't see?
> 
> I kind of this it like this:
> 
> The test site would be a bunch of small pages that test particular features of
> the CMS. Then you can have a pytest fixture that produces different page names
> for each test (see here:
> https://pytest.org/latest/fixture.html#parametrizing-a-fixture) and two
> dependent fixtures, one that runs that page through CMS and produces the
output
> and another that reads the "correct" output from a file. And the test would be
> something like:
> 
> def test_static_rendering(rendered_page, approved_page):
>     assert rendered_page == approved_page
> 
> Perhaps we can ignore whitespace for comparison, but that's details. Later you
> could add `test_dynamic_rendering`, which would use a running CMS server and
> fetch the page via web, but don't worry about it for now.
> 
> Then to get it running you need some way to produce the approved rendered
pages.
> You can just run the CMS over the test pages for that or (this might be better
> in the long term, but don't worry if it seems like too many new things at the
> same time) use something like this framework:
> https://github.com/approvals/ApprovalTests.Python
> 
> How does this sound?

Thanks for these suggestions, I think that I agree with you as well. As long as
I make sure the
test suite runs both dynamic and static tests. For now I will run the cms over
the test site
just to get it functional and if that doesn't take too long I will take a stab
at the approval 
tests thing you posted.

[Jon Sonesen, 2016-06-03 04:59:09]

On 2016/06/02 18:13:54, Jon Sonesen wrote:
> On 2016/06/02 11:44:44, Vasily Kuznetsov wrote:
> > On 2016/06/02 01:21:03, Jon Sonesen wrote:
> > > On 2016/06/01 02:36:51, Jon Sonesen wrote:
> > > > The answer to do the static comparisons may be a subrepo,
> > > > but it is considered a feature of last resort:
> > > > https://www.mercurial-scm.org/wiki/FeaturesOfLastResort
> > > > and should be avoided, any ideas about how to handle this are
appreciated.
> I
> > > was
> > > > thinking that 
> > > > maybe since the content of that subrepo should not change it isn't too
> bad.
> > > But
> > > > that is just one way of looking at it
> > > > I am happy to talk about it.
> > > 
> > > Additionally, we could just have the tet site live in a different repo
that
> a
> > > path can be provided for with the test command
> > 
> > I'm not sure why you want to have the test site in a different repo. It's
part
> > of the test suite of the CMS, so it seems logical to put it in the same
repo.
> Or
> > is there some other reason that I don't see?
> > 
> > I kind of this it like this:
> > 
> > The test site would be a bunch of small pages that test particular features
of
> > the CMS. Then you can have a pytest fixture that produces different page
names
> > for each test (see here:
> > https://pytest.org/latest/fixture.html#parametrizing-a-fixture) and two
> > dependent fixtures, one that runs that page through CMS and produces the
> output
> > and another that reads the "correct" output from a file. And the test would
be
> > something like:
> > 
> > def test_static_rendering(rendered_page, approved_page):
> >     assert rendered_page == approved_page
> > 
> > Perhaps we can ignore whitespace for comparison, but that's details. Later
you
> > could add `test_dynamic_rendering`, which would use a running CMS server and
> > fetch the page via web, but don't worry about it for now.
> > 
> > Then to get it running you need some way to produce the approved rendered
> pages.
> > You can just run the CMS over the test pages for that or (this might be
better
> > in the long term, but don't worry if it seems like too many new things at
the
> > same time) use something like this framework:
> > https://github.com/approvals/ApprovalTests.Python
> > 
> > How does this sound?
> 
> Thanks for these suggestions, I think that I agree with you as well. As long
as
> I make sure the
> test suite runs both dynamic and static tests. For now I will run the cms over
> the test site
> just to get it functional and if that doesn't take too long I will take a stab
> at the approval 
> tests thing you posted.

I believe I found a bug in the sources.py file in the
MercurialSource.has_file(self, filename)
and the MercurialSource.read_file(self, filename) methods. At least when running
this I was receiving a
key error claiming settings.ini did not exist, I experienced this previously 
when testing web.eyeo.com.
This continued to be a problem for me but I just used the test server which
worked fine. Here is the output 
from a pdb.settrace() session:

(Pdb) self._archive.filelist[5].filename 
'settings.ini'
(Pdb) self.has_file('settings.ini')
False
(Pdb) self.has_file('./settings.ini') 
False

At this point I looked at the code in sources.py and found that
has_file uses the _archive.getinfo(self, filename) method and relies on a
KeyError to 
determine whether that file exists.Below is the line from the has_file method
being ran
in pdb:

(Pdb) self._archive.getinfo('./settings.ini') 
*** KeyError: "There is no item named './settings.ini' in the archive"

But I noticed the addition of a relative path which seemed like a first thing to
remove
since those can cause trouble and ran this:

(Pdb) self._archive.getinfo('settings.ini')
<zipfile.ZipInfo object at 0x10d6608e8>

So, this appears to be a legitimate issue where there should not be relative
paths prefixing
filenames, which in looking at it makes sense that those would cause problems.
Please feel free to 
test this and tell me if I am completely wrong (it could easily be that I am on
OSX or something dumb)

Thanks

[Sebastian Noack, 2016-06-03 09:48:58]

On 2016/06/03 04:59:09, Jon Sonesen wrote:
> I believe I found a bug in the sources.py file in the
> MercurialSource.has_file(self, filename)
> and the MercurialSource.read_file(self, filename) methods. At least when
running
> this I was receiving a
> key error claiming settings.ini did not exist, I experienced this previously 
> when testing http://web.eyeo.com.
> This continued to be a problem for me but I just used the test server which
> worked fine. Here is the output 
> from a pdb.settrace() session:
> 
> (Pdb) self._archive.filelist[5].filename 
> 'settings.ini'
> (Pdb) self.has_file('settings.ini')
> False
> (Pdb) self.has_file('./settings.ini') 
> False
> 
> At this point I looked at the code in sources.py and found that
> has_file uses the _archive.getinfo(self, filename) method and relies on a
> KeyError to 
> determine whether that file exists.Below is the line from the has_file method
> being ran
> in pdb:
> 
> (Pdb) self._archive.getinfo('./settings.ini') 
> *** KeyError: "There is no item named './settings.ini' in the archive"
> 
> But I noticed the addition of a relative path which seemed like a first thing
to
> remove
> since those can cause trouble and ran this:
> 
> (Pdb) self._archive.getinfo('settings.ini')
> <zipfile.ZipInfo object at 0x10d6608e8>
> 
> So, this appears to be a legitimate issue where there should not be relative
> paths prefixing
> filenames, which in looking at it makes sense that those would cause problems.
> Please feel free to 
> test this and tell me if I am completely wrong (it could easily be that I am
on
> OSX or something dumb)
> 
> Thanks

How did you invoke static file generation? I cannot reproduce this issue when I
do it as it's documented in README.md. Both of the following command lines work
for me:

~/src/cms $ python -m cms.bin.generate_static_pages ~/src/web.eyeo.com
/tmp/web.eyeo.com.out
~/src/web.eyeo.com $ PYTHONPATH=~/src/cms python -m
cms.bin.generate_static_pages . /tmp/web.eyeo.com.out

I'm on Linux though. But given that Wladimir, who uses OS X too, initially wrote
this code, I'm sure that it was used to work there as well. A regression isn't
impossible of course. But I want to make sure that it's not just a usage error.

[Vasily Kuznetsov, 2016-06-03 14:41:23]

On 2016/06/03 09:48:58, Sebastian Noack wrote:
> On 2016/06/03 04:59:09, Jon Sonesen wrote:
> > I believe I found a bug in the sources.py file in the
> > MercurialSource.has_file(self, filename)
> > and the MercurialSource.read_file(self, filename) methods. At least when
> running
> > this I was receiving a
> > key error claiming settings.ini did not exist, I experienced this previously

> > when testing http://web.eyeo.com.
> > This continued to be a problem for me but I just used the test server which
> > worked fine. Here is the output 
> > from a pdb.settrace() session:
> > 
> > (Pdb) self._archive.filelist[5].filename 
> > 'settings.ini'
> > (Pdb) self.has_file('settings.ini')
> > False
> > (Pdb) self.has_file('./settings.ini') 
> > False
> > 
> > At this point I looked at the code in sources.py and found that
> > has_file uses the _archive.getinfo(self, filename) method and relies on a
> > KeyError to 
> > determine whether that file exists.Below is the line from the has_file
method
> > being ran
> > in pdb:
> > 
> > (Pdb) self._archive.getinfo('./settings.ini') 
> > *** KeyError: "There is no item named './settings.ini' in the archive"
> > 
> > But I noticed the addition of a relative path which seemed like a first
thing
> to
> > remove
> > since those can cause trouble and ran this:
> > 
> > (Pdb) self._archive.getinfo('settings.ini')
> > <zipfile.ZipInfo object at 0x10d6608e8>
> > 
> > So, this appears to be a legitimate issue where there should not be relative
> > paths prefixing
> > filenames, which in looking at it makes sense that those would cause
problems.
> > Please feel free to 
> > test this and tell me if I am completely wrong (it could easily be that I am
> on
> > OSX or something dumb)
> > 
> > Thanks
> 
> How did you invoke static file generation? I cannot reproduce this issue when
I
> do it as it's documented in README.md. Both of the following command lines
work
> for me:
> 
> ~/src/cms $ python -m cms.bin.generate_static_pages ~/src/web.eyeo.com
> /tmp/web.eyeo.com.out
> ~/src/web.eyeo.com $ PYTHONPATH=~/src/cms python -m
> cms.bin.generate_static_pages . /tmp/web.eyeo.com.out
> 
> I'm on Linux though. But given that Wladimir, who uses OS X too, initially
wrote
> this code, I'm sure that it was used to work there as well. A regression isn't
> impossible of course. But I want to make sure that it's not just a usage
error.

This seems to be an OS-specific thing. For me on OS X it breaks the same way as
for Jon and in a Debian VM, this part works and it breaks later, trying to write
to '/var/cache/' (not sure why the code assumes we have rights to write to
'/var/cache', I suppose in production we do). For some reason `hg archive`
ignores `-p .` on OS X but adds the './' prefix to all file names on Linux. I'm
not sure what the original purpose of `-p .` is, but if we remove it or replace
'.' with arbitrary other name, it seems that things will work.

[Jon Sonesen, 2016-06-03 15:13:12]

On 2016/06/03 14:41:23, Vasily Kuznetsov wrote:
> On 2016/06/03 09:48:58, Sebastian Noack wrote:
> > On 2016/06/03 04:59:09, Jon Sonesen wrote:
> > > I believe I found a bug in the sources.py file in the
> > > MercurialSource.has_file(self, filename)
> > > and the MercurialSource.read_file(self, filename) methods. At least when
> > running
> > > this I was receiving a
> > > key error claiming settings.ini did not exist, I experienced this
previously
> 
> > > when testing http://web.eyeo.com.
> > > This continued to be a problem for me but I just used the test server
which
> > > worked fine. Here is the output 
> > > from a pdb.settrace() session:
> > > 
> > > (Pdb) self._archive.filelist[5].filename 
> > > 'settings.ini'
> > > (Pdb) self.has_file('settings.ini')
> > > False
> > > (Pdb) self.has_file('./settings.ini') 
> > > False
> > > 
> > > At this point I looked at the code in sources.py and found that
> > > has_file uses the _archive.getinfo(self, filename) method and relies on a
> > > KeyError to 
> > > determine whether that file exists.Below is the line from the has_file
> method
> > > being ran
> > > in pdb:
> > > 
> > > (Pdb) self._archive.getinfo('./settings.ini') 
> > > *** KeyError: "There is no item named './settings.ini' in the archive"
> > > 
> > > But I noticed the addition of a relative path which seemed like a first
> thing
> > to
> > > remove
> > > since those can cause trouble and ran this:
> > > 
> > > (Pdb) self._archive.getinfo('settings.ini')
> > > <zipfile.ZipInfo object at 0x10d6608e8>
> > > 
> > > So, this appears to be a legitimate issue where there should not be
relative
> > > paths prefixing
> > > filenames, which in looking at it makes sense that those would cause
> problems.
> > > Please feel free to 
> > > test this and tell me if I am completely wrong (it could easily be that I
am
> > on
> > > OSX or something dumb)
> > > 
> > > Thanks
> > 
> > How did you invoke static file generation? I cannot reproduce this issue
when
> I
> > do it as it's documented in README.md. Both of the following command lines
> work
> > for me:
> > 
> > ~/src/cms $ python -m cms.bin.generate_static_pages ~/src/web.eyeo.com
> > /tmp/web.eyeo.com.out
> > ~/src/web.eyeo.com $ PYTHONPATH=~/src/cms python -m
> > cms.bin.generate_static_pages . /tmp/web.eyeo.com.out
> > 
> > I'm on Linux though. But given that Wladimir, who uses OS X too, initially
> wrote
> > this code, I'm sure that it was used to work there as well. A regression
isn't
> > impossible of course. But I want to make sure that it's not just a usage
> error.
> 
> This seems to be an OS-specific thing. For me on OS X it breaks the same way
as
> for Jon and in a Debian VM, this part works and it breaks later, trying to
write
> to '/var/cache/' (not sure why the code assumes we have rights to write to
> '/var/cache', I suppose in production we do). For some reason `hg archive`
> ignores `-p .` on OS X but adds the './' prefix to all file names on Linux.
I'm
> not sure what the original purpose of `-p .` is, but if we remove it or
replace
> '.' with arbitrary other name, it seems that things will work.

Perhaps the cms lives on the build server, so that is why it is meant to behave
this way
and also, maybe Wladimir intended the static page generation to run on whichever
production server it lives on.
So can we safely add handling based on the OS? Or does it seem a bit overkill
since the cms static page generation
is meant to be ran on a specific server? Also, can we be sure that changing that
part of the hg call wont cause
a regression on the build server?

[Jon Sonesen, 2016-06-03 15:13:49]

On 2016/06/03 15:13:12, Jon Sonesen wrote:
> On 2016/06/03 14:41:23, Vasily Kuznetsov wrote:
> > On 2016/06/03 09:48:58, Sebastian Noack wrote:
> > > On 2016/06/03 04:59:09, Jon Sonesen wrote:
> > > > I believe I found a bug in the sources.py file in the
> > > > MercurialSource.has_file(self, filename)
> > > > and the MercurialSource.read_file(self, filename) methods. At least when
> > > running
> > > > this I was receiving a
> > > > key error claiming settings.ini did not exist, I experienced this
> previously
> > 
> > > > when testing http://web.eyeo.com.
> > > > This continued to be a problem for me but I just used the test server
> which
> > > > worked fine. Here is the output 
> > > > from a pdb.settrace() session:
> > > > 
> > > > (Pdb) self._archive.filelist[5].filename 
> > > > 'settings.ini'
> > > > (Pdb) self.has_file('settings.ini')
> > > > False
> > > > (Pdb) self.has_file('./settings.ini') 
> > > > False
> > > > 
> > > > At this point I looked at the code in sources.py and found that
> > > > has_file uses the _archive.getinfo(self, filename) method and relies on
a
> > > > KeyError to 
> > > > determine whether that file exists.Below is the line from the has_file
> > method
> > > > being ran
> > > > in pdb:
> > > > 
> > > > (Pdb) self._archive.getinfo('./settings.ini') 
> > > > *** KeyError: "There is no item named './settings.ini' in the archive"
> > > > 
> > > > But I noticed the addition of a relative path which seemed like a first
> > thing
> > > to
> > > > remove
> > > > since those can cause trouble and ran this:
> > > > 
> > > > (Pdb) self._archive.getinfo('settings.ini')
> > > > <zipfile.ZipInfo object at 0x10d6608e8>
> > > > 
> > > > So, this appears to be a legitimate issue where there should not be
> relative
> > > > paths prefixing
> > > > filenames, which in looking at it makes sense that those would cause
> > problems.
> > > > Please feel free to 
> > > > test this and tell me if I am completely wrong (it could easily be that
I
> am
> > > on
> > > > OSX or something dumb)
> > > > 
> > > > Thanks
> > > 
> > > How did you invoke static file generation? I cannot reproduce this issue
> when
> > I
> > > do it as it's documented in README.md. Both of the following command lines
> > work
> > > for me:
> > > 
> > > ~/src/cms $ python -m cms.bin.generate_static_pages ~/src/web.eyeo.com
> > > /tmp/web.eyeo.com.out
> > > ~/src/web.eyeo.com $ PYTHONPATH=~/src/cms python -m
> > > cms.bin.generate_static_pages . /tmp/web.eyeo.com.out
> > > 
> > > I'm on Linux though. But given that Wladimir, who uses OS X too, initially
> > wrote
> > > this code, I'm sure that it was used to work there as well. A regression
> isn't
> > > impossible of course. But I want to make sure that it's not just a usage
> > error.
> > 
> > This seems to be an OS-specific thing. For me on OS X it breaks the same way
> as
> > for Jon and in a Debian VM, this part works and it breaks later, trying to
> write
> > to '/var/cache/' (not sure why the code assumes we have rights to write to
> > '/var/cache', I suppose in production we do). For some reason `hg archive`
> > ignores `-p .` on OS X but adds the './' prefix to all file names on Linux.
> I'm
> > not sure what the original purpose of `-p .` is, but if we remove it or
> replace
> > '.' with arbitrary other name, it seems that things will work.
> 
> Perhaps the cms lives on the build server, so that is why it is meant to
behave
> this way
> and also, maybe Wladimir intended the static page generation to run on
whichever
> production server it lives on.
> So can we safely add handling based on the OS? Or does it seem a bit overkill
> since the cms static page generation
> is meant to be ran on a specific server? Also, can we be sure that changing
that
> part of the hg call wont cause
> a regression on the build server?

Also, I forgot to mention that the code prefixes the file names explicitly in
the has_file and read_file methods.

[Vasily Kuznetsov, 2016-06-03 15:39:53]

On 2016/06/03 15:13:49, Jon Sonesen wrote:

> > Perhaps the cms lives on the build server, so that is why it is meant to
> behave
> > this way
> > and also, maybe Wladimir intended the static page generation to run on
> whichever
> > production server it lives on.
> > So can we safely add handling based on the OS? Or does it seem a bit
overkill
> > since the cms static page generation
> > is meant to be ran on a specific server? Also, can we be sure that changing
> that
> > part of the hg call wont cause
> > a regression on the build server?

I think it would be better to make it work on OS X as well. I just checked and
`-p` option seems to be necessary, otherwise `hg archive` prepends the path to
the repository to the items in the archive and it also breaks. We still have an
option to replace `-p .` with `-p whatever` to make it behave consistently
across platforms. I suppose if we end up pushing these changes together with the
tests, we're unlikely to break things. What do you think, Sebastian?

[Sebastian Noack, 2016-06-03 16:16:02]

On 2016/06/03 14:41:23, Vasily Kuznetsov wrote:
> in a Debian VM, this part works and it breaks later, trying to write
> to '/var/cache/' (not sure why the code assumes we have rights to write to
> '/var/cache', I suppose in production we do).

Note that the cache dir is only used by web.adblockplus.org at the moment. Also
note that when running the development server a cache directory is created in
the website source directory. However, in production there is no directory (data
are read from a remote hg repo). So when generating the static content, we have
to resort to the global temporary directory. If that is an issue for running
tests, we can just mock MercurialSource.get_cache_dir(), unless you have a
better idea?

On 2016/06/03 15:39:53, Vasily Kuznetsov wrote:
> I think it would be better to make it work on OS X as well. I just checked and
> `-p` option seems to be necessary, otherwise `hg archive` prepends the path to
> the repository to the items in the archive and it also breaks. We still have
an
> option to replace `-p .` with `-p whatever` to make it behave consistently
> across platforms. I suppose if we end up pushing these changes together with
the
> tests, we're unlikely to break things. What do you think, Sebastian?

Yes, I agree that we should make this work on OS X. Otherwise, a fair amount of
people working on the cms cannot run the tests, which would render the tests
quite useless. Not sure which is the best way to fix it though. Can you please
check with Wladimir?

[Wladimir Palant, 2016-06-03 22:35:21]

On 2016/06/03 15:39:53, Vasily Kuznetsov wrote:
> I think it would be better to make it work on OS X as well. I just checked and
> `-p` option seems to be necessary, otherwise `hg archive` prepends the path to
> the repository to the items in the archive and it also breaks. We still have
an
> option to replace `-p .` with `-p whatever` to make it behave consistently
> across platforms

Yes, the idea behind passing in -p flag was having a fixed prefix rather than
the default one which depends on the revision. I can confirm however that `-p .`
omits the prefix altogether for me, this behavior was introduced in Mercurial
3.3.3 (see https://bz.mercurial-scm.org/show_bug.cgi?id=4634). So we should
simply choose another fixed prefix, e.g. `-p root`.

[Jon Sonesen, 2016-06-03 23:32:22]

On 2016/06/03 22:35:21, Wladimir Palant wrote:
> On 2016/06/03 15:39:53, Vasily Kuznetsov wrote:
> > I think it would be better to make it work on OS X as well. I just checked
and
> > `-p` option seems to be necessary, otherwise `hg archive` prepends the path
to
> > the repository to the items in the archive and it also breaks. We still have
> an
> > option to replace `-p .` with `-p whatever` to make it behave consistently
> > across platforms
> 
> Yes, the idea behind passing in -p flag was having a fixed prefix rather than
> the default one which depends on the revision. I can confirm however that `-p
.`
> omits the prefix altogether for me, this behavior was introduced in Mercurial
> 3.3.3 (see https://bz.mercurial-scm.org/show_bug.cgi?id=4634). So we should
> simply choose another fixed prefix, e.g. `-p root`.

Ok, I will implement '-p root' as the prefix accordingly. Thanks for getting
back to us.

[Wladimir Palant, 2016-06-04 09:50:41]

On 2016/06/03 23:32:22, Jon Sonesen wrote:
> Ok, I will implement '-p root' as the prefix accordingly. Thanks for getting
> back to us.

Note that no prefix at all would be even better but IMHO there is no way to
achieve that in older Mercurial versions.

[Jon Sonesen, 2016-06-04 16:44:06]

On 2016/06/04 09:50:41, Wladimir Palant wrote:
> On 2016/06/03 23:32:22, Jon Sonesen wrote:
> > Ok, I will implement '-p root' as the prefix accordingly. Thanks for getting
> > back to us.
> 
> Note that no prefix at all would be even better but IMHO there is no way to
> achieve that in older Mercurial versions.

We could always add hg version checks that change the prefix accordingly if we
*really* wanted too

[Jon Sonesen, 2016-06-20 22:16:47]

here are some changes i made, implenting the static generation fixture and some
others

[Jon Sonesen, 2016-06-20 22:26:41]

here are some changes i made, implementing the static generation fixture and
some others

[Jon Sonesen, 2016-06-28 15:23:50]

adds dynamic test generator

[Jon Sonesen, 2016-06-28 15:27:35]

adds dynamic test generator

[Jon Sonesen, 2016-06-28 15:32:22]

adds dynamic test generator

[Vasily Kuznetsov, 2016-07-01 14:26:16]

Good start. Below are some suggestions on how I think we could better organise
the test:

I think it's better to not separate the tests into static and dynamic ones.
Ideally we'd like to have universal tests that check specific aspects of CMS
output that we'd like to parametrise using a session-scoped fixture that can
retrieve generated pages statically or dynamically depending on the mode. The
way it would work is that you'd set up the following fixtures (all session
scoped):

- source_site (tmpdir_factory): create a root directory for the site, using
tmpdir_factory, make a repo out of it, copy all site content into it and commit,
return the directory itself.
- cms_output (source_site, tmpdir_factory): create a temp directory for CMS
output, run CMS over source_site writing the output into it, return the
directory.
- cms_server (source_size): run a dynamic CMS on some free port on localhost,
return the port number.
- get_page (cms_output, cms_server, request): parametrised fixture that is a
function that returns the page either from cms_output or fetches it via HTTP
depending on the parameter.
- you could also have fixtures for list of input or output pages as you do now
if they're helpful.

Then the tests can depend on `get_page` fixture use it to get the proper page
and check that it looks right. Because of parametrisation of `get_page`, each
test would be run twice in static and dynamic mode.

Once any tests are running, connect them to tox.ini so the reviewers can easily
reproduce the runs with the same environment. I think the current content of
[testenv] should be moved to a tox environment called 'lint' or something like
that and [testenv] should contain the actual test run.

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
File cms/tests/dynamic_tests/conftest.py (right):

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
cms/tests/dynamic_tests/conftest.py:4: SOURCEPATH = 'cms/tests/test_site/pages/'
It's better to not use relative paths like this to not depend on where the tests
are run from. You can use __file__ global, os.path.dirname and os.path.join to
get to where you want.

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
cms/tests/dynamic_tests/conftest.py:14: if hasattr(cls.obj, 'run_test_server'):
Why not just run the test server in this fixture? Then we wouldn't need the
classes and `item.getparent`, which I think is not really a documented feature
that we can safely rely on.

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
File cms/tests/dynamic_tests/test_dynamic.py (right):

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
cms/tests/dynamic_tests/test_dynamic.py:17: url =
'http://localhost:5000/root/{}'\
This code would be encapsulated in the `get_page` fixture.

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/static_te...
File cms/tests/static_tests/conftest.py (right):

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/static_te...
cms/tests/static_tests/conftest.py:10: def static_session_setup(request):
Here again, it seems that we don't need the test class and the setup could be
just done by this fixture. It's also better to use a temporary directory for the
repo, then we don't need to worry about the cleanup.

https://codereview.adblockplus.org/29345468/diff/29347124/pytest.ini
File pytest.ini (right):

https://codereview.adblockplus.org/29345468/diff/29347124/pytest.ini#newcode1
pytest.ini:1: [pytest]
Can you put pytest options into tox.ini? Would be fewer files overall.

[Sebastian Noack, 2016-07-01 14:56:24]

On 2016/07/01 14:26:16, Vasily Kuznetsov wrote:
> Good start. Below are some suggestions on how I think we could better organise
> the test:
> 
> I think it's better to not separate the tests into static and dynamic ones.
> Ideally we'd like to have universal tests that check specific aspects of CMS
> output that we'd like to parametrise using a session-scoped fixture that can
> retrieve generated pages statically or dynamically depending on the mode. The
> way it would work is that you'd set up the following fixtures (all session
> scoped):
> 
> - source_site (tmpdir_factory): create a root directory for the site, using
> tmpdir_factory, make a repo out of it, copy all site content into it and
commit,
> return the directory itself.
> - cms_output (source_site, tmpdir_factory): create a temp directory for CMS
> output, run CMS over source_site writing the output into it, return the
> directory.
> - cms_server (source_size): run a dynamic CMS on some free port on localhost,
> return the port number.
> - get_page (cms_output, cms_server, request): parametrised fixture that is a
> function that returns the page either from cms_output or fetches it via HTTP
> depending on the parameter.
> - you could also have fixtures for list of input or output pages as you do now
> if they're helpful.
> 
> Then the tests can depend on `get_page` fixture use it to get the proper page
> and check that it looks right. Because of parametrisation of `get_page`, each
> test would be run twice in static and dynamic mode.

When Jon and I discussed it yesterday, we came up with following approach:

1. Create a directory with a file containing the expected output per page.
2. Write a fixture that provides each page and it's expected output which is
read from those files.
3. Write one test for the development server and one for the static page
generation, both using that fixture.

What do you think?

> I think the current content of [testenv] should be moved
> to a tox environment called 'lint' or something like
> that and [testenv] should contain the actual test run.

Why? This would be inconsistent with how we use tox everywhere else. Curretnly,
we always run flake8 along tests in the same environment, which IMO makes more
sense anyway, as it makes sure that flake8 isn't skipped and passes on all
supported Python versions.

https://codereview.adblockplus.org/29345468/diff/29347124/.hgignore
File .hgignore (right):

https://codereview.adblockplus.org/29345468/diff/29347124/.hgignore#newcode8
.hgignore:8: 
Did you accidentally added a blank line here?

https://codereview.adblockplus.org/29345468/diff/29347124/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29345468/diff/29347124/cms/sources.py#newc...
cms/sources.py:61: path = '/{}/{}'.format(locale, page)
As mentioned in the other review, please remove those unrelated changes from
this review and create a separate review for cleaning up the string formatting.

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
File cms/tests/dynamic_tests/conftest.py (right):

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
cms/tests/dynamic_tests/conftest.py:20: request.addfinalizer(dynamic_tear_down)
Note that the wrapper function is redundant. You can just call:

  request.addfinalizer(p.terminate)

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
File cms/tests/dynamic_tests/test_dynamic.py (right):

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
cms/tests/dynamic_tests/test_dynamic.py:18: .format(os.path.splitext(page))
I wonder whether this code behaves as intended since os.path.splitext() returns
a tuple? Also, if just concatenating two strings, use the + operator.

https://codereview.adblockplus.org/29345468/diff/29347124/pytest.ini
File pytest.ini (right):

https://codereview.adblockplus.org/29345468/diff/29347124/pytest.ini#newcode1
pytest.ini:1: [pytest]
On 2016/07/01 14:26:16, Vasily Kuznetsov wrote:
> Can you put pytest options into tox.ini? Would be fewer files overall.

Why do we need that option anyway?

[Vasily Kuznetsov, 2016-07-01 15:18:02]

On 2016/07/01 14:56:24, Sebastian Noack wrote:
> On 2016/07/01 14:26:16, Vasily Kuznetsov wrote:
> ...
> When Jon and I discussed it yesterday, we came up with following approach:
> 
> 1. Create a directory with a file containing the expected output per page.
> 2. Write a fixture that provides each page and it's expected output which is
> read from those files.
> 3. Write one test for the development server and one for the static page
> generation, both using that fixture.
> 
> What do you think?

Yeah, this also makes sense. I basically just want to avoid duplication, so any
way that achieves it without too much hacking is good.

> 
> > I think the current content of [testenv] should be moved
> > to a tox environment called 'lint' or something like
> > that and [testenv] should contain the actual test run.
> 
> Why? This would be inconsistent with how we use tox everywhere else.
Curretnly,
> we always run flake8 along tests in the same environment, which IMO makes more
> sense anyway, as it makes sure that flake8 isn't skipped and passes on all
> supported Python versions.
> 

Yes, you're right. Then we just add the test run to [testenv]. Sorry for the
confusion.

> > Can you put pytest options into tox.ini? Would be fewer files overall.
> 
> Why do we need that option anyway?

Yeah, I'm also not sure we do, probably no. Or do we, Jon?

[Jon Sonesen, 2016-07-01 15:19:40]

On 2016/07/01 15:18:02, Vasily Kuznetsov wrote:
> On 2016/07/01 14:56:24, Sebastian Noack wrote:
> > On 2016/07/01 14:26:16, Vasily Kuznetsov wrote:
> > ...
> > When Jon and I discussed it yesterday, we came up with following approach:
> > 
> > 1. Create a directory with a file containing the expected output per page.
> > 2. Write a fixture that provides each page and it's expected output which is
> > read from those files.
> > 3. Write one test for the development server and one for the static page
> > generation, both using that fixture.
> > 
> > What do you think?
> 
> Yeah, this also makes sense. I basically just want to avoid duplication, so
any
> way that achieves it without too much hacking is good.
> 
> > 
> > > I think the current content of [testenv] should be moved
> > > to a tox environment called 'lint' or something like
> > > that and [testenv] should contain the actual test run.
> > 
> > Why? This would be inconsistent with how we use tox everywhere else.
> Curretnly,
> > we always run flake8 along tests in the same environment, which IMO makes
more
> > sense anyway, as it makes sure that flake8 isn't skipped and passes on all
> > supported Python versions.
> > 
> 
> Yes, you're right. Then we just add the test run to [testenv]. Sorry for the
> confusion.
> 
> > > Can you put pytest options into tox.ini? Would be fewer files overall.
> > 
> > Why do we need that option anyway?
> 
> Yeah, I'm also not sure we do, probably no. Or do we, Jon?

The option is to turn off caching, which in retrospect seems useless and may
complicate things later since the cache allows you to run only failed tests etc
etc.
 I will remove it.

[Jon Sonesen, 2016-07-01 15:58:14]

https://codereview.adblockplus.org/29345468/diff/29347124/.hgignore
File .hgignore (right):

https://codereview.adblockplus.org/29345468/diff/29347124/.hgignore#newcode8
.hgignore:8: 
On 2016/07/01 14:56:23, Sebastian Noack wrote:
> Did you accidentally added a blank line here?

yes I will also address this.

https://codereview.adblockplus.org/29345468/diff/29347124/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29345468/diff/29347124/cms/sources.py#newc...
cms/sources.py:61: path = '/{}/{}'.format(locale, page)
On 2016/07/01 14:56:23, Sebastian Noack wrote:
> As mentioned in the other review, please remove those unrelated changes from
> this review and create a separate review for cleaning up the string
formatting.

This is already handled. This patch set was uploaded a few days ago.

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
File cms/tests/dynamic_tests/conftest.py (right):

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
cms/tests/dynamic_tests/conftest.py:4: SOURCEPATH = 'cms/tests/test_site/pages/'
On 2016/07/01 14:26:15, Vasily Kuznetsov wrote:
> It's better to not use relative paths like this to not depend on where the
tests
> are run from. You can use __file__ global, os.path.dirname and os.path.join to
> get to where you want.

Thanks for the tip I will get that fixed up.

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
cms/tests/dynamic_tests/conftest.py:14: if hasattr(cls.obj, 'run_test_server'):
On 2016/07/01 14:26:15, Vasily Kuznetsov wrote:
> Why not just run the test server in this fixture? Then we wouldn't need the
> classes and `item.getparent`, which I think is not really a documented feature
> that we can safely rely on.

Yeah I agree, I am going to include the setup/teardown code for both static and
dynamic tests in this fixture.

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
cms/tests/dynamic_tests/conftest.py:20: request.addfinalizer(dynamic_tear_down)
On 2016/07/01 14:56:23, Sebastian Noack wrote:
> Note that the wrapper function is redundant. You can just call:
> 
>   request.addfinalizer(p.terminate)

Thank you, however once the teardown for static and dynamic are included here
won't that change?

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
File cms/tests/dynamic_tests/test_dynamic.py (right):

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
cms/tests/dynamic_tests/test_dynamic.py:17: url =
'http://localhost:5000/root/{}'\
On 2016/07/01 14:26:15, Vasily Kuznetsov wrote:
> This code would be encapsulated in the `get_page` fixture.

Will do.

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/dynamic_t...
cms/tests/dynamic_tests/test_dynamic.py:18: .format(os.path.splitext(page))
On 2016/07/01 14:56:23, Sebastian Noack wrote:
> I wonder whether this code behaves as intended since os.path.splitext()
returns
> a tuple? Also, if just concatenating two strings, use the + operator.

the test server does return 200, and the page contents and pass the test but it
is still wrong and seems sloppy so I will address it.

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/static_te...
File cms/tests/static_tests/conftest.py (right):

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/static_te...
cms/tests/static_tests/conftest.py:10: def static_session_setup(request):
On 2016/07/01 14:26:16, Vasily Kuznetsov wrote:
> Here again, it seems that we don't need the test class and the setup could be
> just done by this fixture. It's also better to use a temporary directory for
the
> repo, then we don't need to worry about the cleanup.
 I agree. But what do you mean temporary directory for the repo? You mean the
test_site?

https://codereview.adblockplus.org/29345468/diff/29347124/pytest.ini
File pytest.ini (right):

https://codereview.adblockplus.org/29345468/diff/29347124/pytest.ini#newcode1
pytest.ini:1: [pytest]
On 2016/07/01 14:56:23, Sebastian Noack wrote:
> On 2016/07/01 14:26:16, Vasily Kuznetsov wrote:
> > Can you put pytest options into tox.ini? Would be fewer files overall.
> 
> Why do we need that option anyway?

It is going to be removed.

[Vasily Kuznetsov, 2016-07-01 16:47:59]

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/static_te...
File cms/tests/static_tests/conftest.py (right):

https://codereview.adblockplus.org/29345468/diff/29347124/cms/tests/static_te...
cms/tests/static_tests/conftest.py:10: def static_session_setup(request):
On 2016/07/01 15:58:13, Jon Sonesen wrote:
> On 2016/07/01 14:26:16, Vasily Kuznetsov wrote:
> > Here again, it seems that we don't need the test class and the setup could
be
> > just done by this fixture. It's also better to use a temporary directory for
> the
> > repo, then we don't need to worry about the cleanup.
>  I agree. But what do you mean temporary directory for the repo? You mean the
> test_site?

I think it's preferable to create a temporary directory that will be
automatically deleted (using pytest APIs) and then create the source repo inside
of it (create repo, copy stuff from test_site, add, commit) and do all the
static generation to another directory inside of a temporary directory. This is
cleaner approach that in any case doesn't leave testing fragments in the source
tree and leans more on pytest functionality for cleanup, so less code for us.

BTW, for copying files take a look at `shutil.copytree`.

[Jon Sonesen, 2016-07-12 13:43:19]

Here is that updated tests. Consolidated the session code into one module and
addressed comments

[Vasily Kuznetsov, 2016-07-12 15:25:13]

Hi Jon,

The direction is looking very good. Let's add more pages to test different
features.

You can also already add the test run to `tox.ini`. Here's a patch:

diff --git a/tox.ini b/tox.ini
--- a/tox.ini
+++ b/tox.ini
@@ -7,16 +7,24 @@
     cms/bin/generate_static_pages.py : A107,A301,A302,E501
     cms/bin/test_server.py : A101,A107,A302,E501,F401
     cms/bin/translate.py : A107,A301,A302,E501,F821
     cms/converters.py : A107,A108,A201,A206,A302,E501,E711,N802
     cms/sources.py : A104,A107,A108,A206,A302,E501
     cms/utils.py : A107,A302,E501

 [testenv]
+setenv =
+    PYTHONPATH = {toxinidir}
+
 deps =
+    pytest
+    jinja2
+    markdown
     flake8
     flake8-putty
     pep8-naming
    
hg+https://hg.adblockplus.org/codingtools#egg=flake8-abp&subdirectory=flake8-abp
+
 commands =
+    py.test cms/tests
     flake8 cms runserver.py
     flake8 --ignore E501,F821,N802 runserver.spec


More comments on specific lines below:

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
File cms/tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
cms/tests/test_page_outputs.py:16: for (dirpath, dirnames, filenames) in
os.walk(expected_out_path):
You actually don't need the parentheses around `dirpath, dirname, filenames`.

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
cms/tests/test_page_outputs.py:18: with open('{}/{}'.format(dirpath,
output_file)) as f:
Here it's also better to use `os.path.join`.

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
cms/tests/test_page_outputs.py:27: site_dir = tmpdir_factory.mktemp('temp_out')
Maybe better call this `out_dir`. Otherwise `site_dir` gets reassigned with a
value that has different meaning and also different type. That's usually not a
very good practice for maintainability.

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
cms/tests/test_page_outputs.py:32: subprocess.check_call(
This call seems to fit on one line, so there's no need for line wrapping.

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
cms/tests/test_page_outputs.py:46: runpy.run_module(
This one also fits on one line.

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
cms/tests/test_page_outputs.py:52: @pytest.fixture(scope='session')
It seems that with current default version of pytest (2.9.2) this fixture
doesn't work. You'd need to use `yield_fixture` decorator instead. I think this
will be changed in pytest 3.0 and then your code will be ok.

Or does it work for you?

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
cms/tests/test_page_outputs.py:54: p = subprocess.Popen(
This also fits on one line.

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
cms/tests/test_page_outputs.py:65: for (dirpath, dirnames, filenames) in
os.walk(static_output):
This code seems to be duplicated from `get_expected_outputs`. We could extract a
method that reads all the files in a directory into a dict and then use it in
both places.

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
cms/tests/test_page_outputs.py:72:
@pytest.mark.parametrize('filename,expected_output', expected_outputs.items())
It seems that we're not using `expected_output` as a dictionary anywhere so if
that will continue being the case, probably better convert it to a list of
tuples and get rid of `.items()` here and in the decorator for the next test.

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_site...
File cms/tests/test_site/pages/filter.tmpl (right):

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_site...
cms/tests/test_site/pages/filter.tmpl:3: {{- test|foo_converter }}
You can just inline the value of `test` like this `{{- 'test'|foo_converter }}`,
then we don't need the previous line.

[Jon Sonesen, 2016-07-13 10:52:05]

On 2016/07/12 15:25:13, Vasily Kuznetsov wrote:
> Hi Jon,
> 
> The direction is looking very good. Let's add more pages to test different
> features.
> 
> You can also already add the test run to `tox.ini`. Here's a patch:
> 
> diff --git a/tox.ini b/tox.ini
> --- a/tox.ini
> +++ b/tox.ini
> @@ -7,16 +7,24 @@
>      cms/bin/generate_static_pages.py : A107,A301,A302,E501
>      cms/bin/test_server.py : A101,A107,A302,E501,F401
>      cms/bin/translate.py : A107,A301,A302,E501,F821
>      cms/converters.py : A107,A108,A201,A206,A302,E501,E711,N802
>      cms/sources.py : A104,A107,A108,A206,A302,E501
>      cms/utils.py : A107,A302,E501
> 
>  [testenv]
> +setenv =
> +    PYTHONPATH = {toxinidir}
> +
>  deps =
> +    pytest
> +    jinja2
> +    markdown
>      flake8
>      flake8-putty
>      pep8-naming
>     
>
hg+https://hg.adblockplus.org/codingtools#egg=flake8-abp&subdirectory=flake8-abp
> +
>  commands =
> +    py.test cms/tests
>      flake8 cms runserver.py
>      flake8 --ignore E501,F821,N802 runserver.spec
> 
> 
> More comments on specific lines below:
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> File cms/tests/test_page_outputs.py (right):
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> cms/tests/test_page_outputs.py:16: for (dirpath, dirnames, filenames) in
> os.walk(expected_out_path):
> You actually don't need the parentheses around `dirpath, dirname, filenames`.
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> cms/tests/test_page_outputs.py:18: with open('{}/{}'.format(dirpath,
> output_file)) as f:
> Here it's also better to use `os.path.join`.
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> cms/tests/test_page_outputs.py:27: site_dir =
tmpdir_factory.mktemp('temp_out')
> Maybe better call this `out_dir`. Otherwise `site_dir` gets reassigned with a
> value that has different meaning and also different type. That's usually not a
> very good practice for maintainability.
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> cms/tests/test_page_outputs.py:32: subprocess.check_call(
> This call seems to fit on one line, so there's no need for line wrapping.
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> cms/tests/test_page_outputs.py:46: runpy.run_module(
> This one also fits on one line.
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> cms/tests/test_page_outputs.py:52: @pytest.fixture(scope='session')
> It seems that with current default version of pytest (2.9.2) this fixture
> doesn't work. You'd need to use `yield_fixture` decorator instead. I think
this
> will be changed in pytest 3.0 and then your code will be ok.
> 
> Or does it work for you?
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> cms/tests/test_page_outputs.py:54: p = subprocess.Popen(
> This also fits on one line.
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> cms/tests/test_page_outputs.py:65: for (dirpath, dirnames, filenames) in
> os.walk(static_output):
> This code seems to be duplicated from `get_expected_outputs`. We could extract
a
> method that reads all the files in a directory into a dict and then use it in
> both places.
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> cms/tests/test_page_outputs.py:72:
> @pytest.mark.parametrize('filename,expected_output', expected_outputs.items())
> It seems that we're not using `expected_output` as a dictionary anywhere so if
> that will continue being the case, probably better convert it to a list of
> tuples and get rid of `.items()` here and in the decorator for the next test.
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_site...
> File cms/tests/test_site/pages/filter.tmpl (right):
> 
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_site...
> cms/tests/test_site/pages/filter.tmpl:3: {{- test|foo_converter }}
> You can just inline the value of `test` like this `{{- 'test'|foo_converter
}}`,
> then we don't need the previous line.

Just uploaded a patch set I think addressing all the comments as well as adding
a global test page :D

[Jon Sonesen, 2016-07-13 10:52:56]

On 2016/07/13 10:52:05, Jon Sonesen wrote:
> On 2016/07/12 15:25:13, Vasily Kuznetsov wrote:
> > Hi Jon,
> > 
> > The direction is looking very good. Let's add more pages to test different
> > features.
> > 
> > You can also already add the test run to `tox.ini`. Here's a patch:
> > 
> > diff --git a/tox.ini b/tox.ini
> > --- a/tox.ini
> > +++ b/tox.ini
> > @@ -7,16 +7,24 @@
> >      cms/bin/generate_static_pages.py : A107,A301,A302,E501
> >      cms/bin/test_server.py : A101,A107,A302,E501,F401
> >      cms/bin/translate.py : A107,A301,A302,E501,F821
> >      cms/converters.py : A107,A108,A201,A206,A302,E501,E711,N802
> >      cms/sources.py : A104,A107,A108,A206,A302,E501
> >      cms/utils.py : A107,A302,E501
> > 
> >  [testenv]
> > +setenv =
> > +    PYTHONPATH = {toxinidir}
> > +
> >  deps =
> > +    pytest
> > +    jinja2
> > +    markdown
> >      flake8
> >      flake8-putty
> >      pep8-naming
> >     
> >
>
hg+https://hg.adblockplus.org/codingtools#egg=flake8-abp&subdirectory=flake8-abp
> > +
> >  commands =
> > +    py.test cms/tests
> >      flake8 cms runserver.py
> >      flake8 --ignore E501,F821,N802 runserver.spec
> > 
> > 
> > More comments on specific lines below:
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > File cms/tests/test_page_outputs.py (right):
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:16: for (dirpath, dirnames, filenames) in
> > os.walk(expected_out_path):
> > You actually don't need the parentheses around `dirpath, dirname,
filenames`.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:18: with open('{}/{}'.format(dirpath,
> > output_file)) as f:
> > Here it's also better to use `os.path.join`.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:27: site_dir =
> tmpdir_factory.mktemp('temp_out')
> > Maybe better call this `out_dir`. Otherwise `site_dir` gets reassigned with
a
> > value that has different meaning and also different type. That's usually not
a
> > very good practice for maintainability.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:32: subprocess.check_call(
> > This call seems to fit on one line, so there's no need for line wrapping.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:46: runpy.run_module(
> > This one also fits on one line.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:52: @pytest.fixture(scope='session')
> > It seems that with current default version of pytest (2.9.2) this fixture
> > doesn't work. You'd need to use `yield_fixture` decorator instead. I think
> this
> > will be changed in pytest 3.0 and then your code will be ok.
> > 
> > Or does it work for you?
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:54: p = subprocess.Popen(
> > This also fits on one line.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:65: for (dirpath, dirnames, filenames) in
> > os.walk(static_output):
> > This code seems to be duplicated from `get_expected_outputs`. We could
extract
> a
> > method that reads all the files in a directory into a dict and then use it
in
> > both places.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:72:
> > @pytest.mark.parametrize('filename,expected_output',
expected_outputs.items())
> > It seems that we're not using `expected_output` as a dictionary anywhere so
if
> > that will continue being the case, probably better convert it to a list of
> > tuples and get rid of `.items()` here and in the decorator for the next
test.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_site...
> > File cms/tests/test_site/pages/filter.tmpl (right):
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_site...
> > cms/tests/test_site/pages/filter.tmpl:3: {{- test|foo_converter }}
> > You can just inline the value of `test` like this `{{- 'test'|foo_converter
> }}`,
> > then we don't need the previous line.
> 
> Just uploaded a patch set I think addressing all the comments as well as
adding
> a global test page :D

Oh...actually I did not add the tox.ini changes vasiliy uploaded.

[Jon Sonesen, 2016-07-13 10:53:00]

On 2016/07/13 10:52:05, Jon Sonesen wrote:
> On 2016/07/12 15:25:13, Vasily Kuznetsov wrote:
> > Hi Jon,
> > 
> > The direction is looking very good. Let's add more pages to test different
> > features.
> > 
> > You can also already add the test run to `tox.ini`. Here's a patch:
> > 
> > diff --git a/tox.ini b/tox.ini
> > --- a/tox.ini
> > +++ b/tox.ini
> > @@ -7,16 +7,24 @@
> >      cms/bin/generate_static_pages.py : A107,A301,A302,E501
> >      cms/bin/test_server.py : A101,A107,A302,E501,F401
> >      cms/bin/translate.py : A107,A301,A302,E501,F821
> >      cms/converters.py : A107,A108,A201,A206,A302,E501,E711,N802
> >      cms/sources.py : A104,A107,A108,A206,A302,E501
> >      cms/utils.py : A107,A302,E501
> > 
> >  [testenv]
> > +setenv =
> > +    PYTHONPATH = {toxinidir}
> > +
> >  deps =
> > +    pytest
> > +    jinja2
> > +    markdown
> >      flake8
> >      flake8-putty
> >      pep8-naming
> >     
> >
>
hg+https://hg.adblockplus.org/codingtools#egg=flake8-abp&subdirectory=flake8-abp
> > +
> >  commands =
> > +    py.test cms/tests
> >      flake8 cms runserver.py
> >      flake8 --ignore E501,F821,N802 runserver.spec
> > 
> > 
> > More comments on specific lines below:
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > File cms/tests/test_page_outputs.py (right):
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:16: for (dirpath, dirnames, filenames) in
> > os.walk(expected_out_path):
> > You actually don't need the parentheses around `dirpath, dirname,
filenames`.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:18: with open('{}/{}'.format(dirpath,
> > output_file)) as f:
> > Here it's also better to use `os.path.join`.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:27: site_dir =
> tmpdir_factory.mktemp('temp_out')
> > Maybe better call this `out_dir`. Otherwise `site_dir` gets reassigned with
a
> > value that has different meaning and also different type. That's usually not
a
> > very good practice for maintainability.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:32: subprocess.check_call(
> > This call seems to fit on one line, so there's no need for line wrapping.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:46: runpy.run_module(
> > This one also fits on one line.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:52: @pytest.fixture(scope='session')
> > It seems that with current default version of pytest (2.9.2) this fixture
> > doesn't work. You'd need to use `yield_fixture` decorator instead. I think
> this
> > will be changed in pytest 3.0 and then your code will be ok.
> > 
> > Or does it work for you?
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:54: p = subprocess.Popen(
> > This also fits on one line.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:65: for (dirpath, dirnames, filenames) in
> > os.walk(static_output):
> > This code seems to be duplicated from `get_expected_outputs`. We could
extract
> a
> > method that reads all the files in a directory into a dict and then use it
in
> > both places.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
> > cms/tests/test_page_outputs.py:72:
> > @pytest.mark.parametrize('filename,expected_output',
expected_outputs.items())
> > It seems that we're not using `expected_output` as a dictionary anywhere so
if
> > that will continue being the case, probably better convert it to a list of
> > tuples and get rid of `.items()` here and in the decorator for the next
test.
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_site...
> > File cms/tests/test_site/pages/filter.tmpl (right):
> > 
> >
>
https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_site...
> > cms/tests/test_site/pages/filter.tmpl:3: {{- test|foo_converter }}
> > You can just inline the value of `test` like this `{{- 'test'|foo_converter
> }}`,
> > then we don't need the previous line.
> 
> Just uploaded a patch set I think addressing all the comments as well as
adding
> a global test page :D

Oh...actually I did not add the tox.ini changes vasiliy uploaded.

[Vasily Kuznetsov, 2016-07-14 12:14:02]

Two small corrections below and then it mostly would look good to me for the
part that we have so far.

Sebastian, what do you think if we'd wrap up this review at current level of
functionality? So Jon would address the whatever outstanding comments, remove
the few unused files and commit this as the "test setup for CMS". Then we'd
continue adding further coverage (translations, etc.) in separate tickets?

https://codereview.adblockplus.org/29345468/diff/29347768/cms/tests/test_page...
File cms/tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29345468/diff/29347768/cms/tests/test_page...
cms/tests/test_page_outputs.py:18: return_data[output_file] = f.read()
Is it intentional that we ignore the directory here? For example `foo/bar` will
overwrite the content of `bar` and later, in the dynamic test, the value will be
compared with the result of the request for `http://localhost:5000/bar`, which
seems incorrect.

https://codereview.adblockplus.org/29345468/diff/29347768/cms/tests/test_site...
File cms/tests/test_site/pages/global.tmpl (right):

https://codereview.adblockplus.org/29345468/diff/29347768/cms/tests/test_site...
cms/tests/test_site/pages/global.tmpl:3: {%- set test = site_globals('test') -%}
Is it important to assign the output of this function to a variable or could we
just do `{{- site_globals('test') }}` instead?

[Jon Sonesen, 2016-07-18 14:48:29]

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
File cms/tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29345468/diff/29347662/cms/tests/test_page...
cms/tests/test_page_outputs.py:52: @pytest.fixture(scope='session')
On 2016/07/12 15:25:12, Vasily Kuznetsov wrote:
> It seems that with current default version of pytest (2.9.2) this fixture
> doesn't work. You'd need to use `yield_fixture` decorator instead. I think
this
> will be changed in pytest 3.0 and then your code will be ok.
> 
> Or does it work for you?
It works but I believe I am still using the pytest-dev so that would explain why
so I will address this.

https://codereview.adblockplus.org/29345468/diff/29347768/cms/tests/test_page...
File cms/tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29345468/diff/29347768/cms/tests/test_page...
cms/tests/test_page_outputs.py:18: return_data[output_file] = f.read()
On 2016/07/14 12:14:00, Vasily Kuznetsov wrote:
> Is it intentional that we ignore the directory here? For example `foo/bar`
will
> overwrite the content of `bar` and later, in the dynamic test, the value will
be
> compared with the result of the request for `http://localhost:5000/bar`, which
> seems incorrect.

The reason we are ignoring the directories is because only the pages directory
contains paths to compare and withing each nested dir I thought the cms just
built the path disregarding those directories

https://codereview.adblockplus.org/29345468/diff/29347768/cms/tests/test_site...
File cms/tests/test_site/pages/global.tmpl (right):

https://codereview.adblockplus.org/29345468/diff/29347768/cms/tests/test_site...
cms/tests/test_site/pages/global.tmpl:3: {%- set test = site_globals('test') -%}
On 2016/07/14 12:14:00, Vasily Kuznetsov wrote:
> Is it important to assign the output of this function to a variable or could
we
> just do `{{- site_globals('test') }}` instead?

I think I can just do what you put. I will change.

[Sebastian Noack, 2016-08-11 10:09:03]

Any reason the "tests" folder is inside the "cms" package? I looked a couple
popular projects Python projects (jinja2, tox, pytest) and all of them put their
testsuite folder under the project root, rather than inside the package, which
actually makes sense.

[Jon Sonesen, 2016-08-21 13:43:14]

On 2016/08/11 10:09:03, Sebastian Noack wrote:
> Any reason the "tests" folder is inside the "cms" package? I looked a couple
> popular projects Python projects (jinja2, tox, pytest) and all of them put
their
> testsuite folder under the project root, rather than inside the package, which
> actually makes sense.

I moved the test folder and updated the tox ini file :D

[Sebastian Noack, 2016-08-21 14:33:10]

https://codereview.adblockplus.org/29345468/diff/29350005/tox.ini
File tox.ini (left):

https://codereview.adblockplus.org/29345468/diff/29350005/tox.ini#oldcode21
tox.ini:21: flake8 cms runserver.py
Mind running flake8 also on the tests directory?

[Jon Sonesen, 2016-08-21 14:36:36]

Adds flake8 run to tests dir

[Jon Sonesen, 2016-08-21 14:37:11]

On 2016/08/21 14:33:10, Sebastian Noack wrote:
> https://codereview.adblockplus.org/29345468/diff/29350005/tox.ini
> File tox.ini (left):
> 
> https://codereview.adblockplus.org/29345468/diff/29350005/tox.ini#oldcode21
> tox.ini:21: flake8 cms runserver.py
> Mind running flake8 also on the tests directory?

 Done.

[Sebastian Noack, 2016-08-21 15:10:06]

LGTM

[Vasily Kuznetsov, 2016-08-22 10:14:53]

LAGTM (Looks almost good to me ;)

https://codereview.adblockplus.org/29345468/diff/29350022/tests/test_site/tem...
File tests/test_site/templates/default.tmpl (right):

https://codereview.adblockplus.org/29345468/diff/29350022/tests/test_site/tem...
tests/test_site/templates/default.tmpl:1: <!DOCTYPE html>
This template seems to be not used by any pages that are tested. Do we actually
need it?

Same for `pages/locale.html` and `pages/foobar/index.html`. They are not used by
the testing suite at the moment. Maybe we can just remove them?

https://codereview.adblockplus.org/29345468/diff/29350022/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29345468/diff/29350022/tox.ini#newcode1
tox.ini:1: [tox]
At the moment this change doesn't apply on top of the last commit (Issue 4342).
It would be nice to rebase the review just so what goes into the eventual commit
is exactly what we've reviewed.

[Jon Sonesen, 2016-08-23 15:34:56]

Adds flake8 run to tests dir

[Jon Sonesen, 2016-08-23 15:36:40]

rebased and removed unused tests

[Jon Sonesen, 2016-08-23 15:40:30]

rebase for flake8 explicit version in tox ini and removes unused tests

[Vasily Kuznetsov, 2016-08-23 16:38:47]

On 2016/08/23 15:40:30, Jon Sonesen wrote:
> rebase for flake8 explicit version in tox ini and removes unused tests

LGTM,

One detail before this is done though:
The last patch set currently includes Sebastian's changes to `tox.ini` related
to issue 4342 (this commit: https://hg.adblockplus.org/cms/rev/ecd76872aefc).
Could you please re-upload the patch set without that commit, so we have a clean
situation where the review will be exactly like the changes that will be pushed.
Thanks and sorry for the bureaucracy ;)

[Jon Sonesen, 2016-08-25 09:05:14]

fixed erroneous upload of patch

[Vasily Kuznetsov, 2016-08-25 09:09:54]

On 2016/08/25 09:05:14, Jon Sonesen wrote:
> fixed erroneous upload of patch

LGTM