Issue 29501558: Issue 5383 - Add tests for the Chrome and Firefox packagers

tlucas

Patch Set 1 * Outsourced n>1-times used fixtures into conftest.py * Adjusted py.test config to ...

July 31, 2017, 12:14 p.m. (2017-07-31 12:14:12 UTC) #1

kzar

Thanks for looking at this. Just so you know Sebastian is away at the moment ...

July 31, 2017, 2:27 p.m. (2017-07-31 14:27:24 UTC) #2

kzar

On 2017/07/31 14:27:24, kzar wrote: > Thanks for looking at this. Just so you know ...

Aug. 2, 2017, 11:28 a.m. (2017-08-02 11:28:40 UTC) #3

tlucas

On 2017/08/02 11:28:40, kzar wrote: > On 2017/07/31 14:27:24, kzar wrote: > > Thanks for ...

Aug. 2, 2017, 11:37 a.m. (2017-08-02 11:37:18 UTC) #4

Vasily Kuznetsov

Hi Tristan, This is pretty impressive! Nice work. I also have lots of comments, see ...

Aug. 3, 2017, 4:52 p.m. (2017-08-03 16:52:33 UTC) #5

tlucas

Hey Vasily, thanks for the review! I mostly agree with your comments, some comments require ...

Aug. 3, 2017, 9:26 p.m. (2017-08-03 21:26:03 UTC) #6

Hey Vasily,

thanks for the review!

I mostly agree with your comments, some comments require further dicussion imho,
please see below - In the meantime, i will change the other things :)

Cheers,
Tristan

https://codereview.adblockplus.org/29501558/diff/29501559/tests/conftest.py
File tests/conftest.py (right):

https://codereview.adblockplus.org/29501558/diff/29501559/tests/conftest.py#n...
tests/conftest.py:29: @pytest.fixture
On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> What's the purpose of this vs. just using `tmpdir`?

Iirc the tmpdir fixture would not have allowed me to have multiple other
fixtures use the same directory when needed - i'll look into this though and
check back with you

https://codereview.adblockplus.org/29501558/diff/29501559/tests/conftest.py#n...
tests/conftest.py:36: def chars():
On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> This name is quite generic. What do you think about renaming this to something
a
> bit more specific, like `str500chars`, to make the fixture easier to find and
> also giving it a docstring?

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
File tests/test_packagerChrome.py (right):

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:18: TR_FA = [True, False]
On 2017/08/03 16:52:29, Vasily Kuznetsov wrote:
> Not sure having this constant is worth it. TR_FA is a bit of a confusing name
> and if you call it TRUE_FALSE, then it's kind of pointless to have the
constant.

This will be gotten rid of by proper parametrization anyway: Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:20: MINIMUM_MANIFEST_JSON = [
On 2017/08/03 16:52:29, Vasily Kuznetsov wrote:
> Isn't this name a bit confusing given that the content of this constant is not
> JSON but a list? Maybe just call it MINIMUM_MANIFEST?

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:21: ('manifest_version', 2),
On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> For consistency with other code, it would be better to indent this list as
> follows:
> 
> FOO = [
>     'bar',
>     'baz',
> ]

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:40: try:
On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> Since this runs in tests, we probably know which branch of this try/except
will
> actually work. Or do we not know it?

We actually do, i just added this try/except before adding pillow to the
tox.ini. Acknowledged

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:50: paths +=
[str(icon_dir.join('abp-{}.png'.format(size)))]
On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> What do you think about rewriting this as follows?
> 
>         img_path = icon_dir.join(...).strpath
>         img.save(img_path)
>         paths.append(img_path)
> 
> Seems like it would be a bit more readable.
> 
> Also, using `some_pypath.strpath` is preferable to `str(some_pythonpath)`
since
> it's a more specific code that uses py.path API and it's also consistent with
> how we're doing it in most other tests.

I agree, this would be a lot more readable.

Regarding the str-comment:
the str(some_pypath) was inspired by the original test_packagerEdge.py content -
which style should i follow here?

Also: Afaik the __str__ method simply returns self.strpath for now and imho the
explicit conversion is more readable - but i wouldn't mind changing that (and
maybe adjust the respective code in test_packagerEdge as well?)

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:96: @pytest.mark.parametrize(
On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> What do you think about creating 3 fixtures instead: `gecko_metadata`,
> `chrome_metadata` and `edge_metadata`? Then the tests could depend on those
> fixtures directly instead of having to copy these `mark.parametrize` and
> `mark.usefixtures` everywhere.

This would also reduce the amount of readMetadata-calls. Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:162: expected = ''.join((
On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> You can actually write this as follows:
> 
>     expected = (
>         '3495802348590234850934850234509'
>         '4909324063490682390462820934680'
>         ...
>     )
> 
> When a bunch of string literals are written one after another like this,
Python
> will just concatenate them into one big constant.

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:180: extensions = ['html', 'js', 'json', 'xml']
On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> What do you think about replacing this with:
> 
>     files = ['foo.html', 'foo.js', 'foo.json', 'foo.xml']
> 
> Then you wouldn't need to prepend 'foo.' to each extension twice in the
> following code and it would become simpler and more readable.

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:184: for devenv in [True, False]:
On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> What do you think about making devenv a parameter to the test:
> 
> @pytest.mark.parametrize('devenv', [True, False])
> def test_get_package_files(srcdir, devenv):
>     ...
> 
> This would convert this test into two test items that might pass or fail
> independently thus producing better granularity of the reporting. And it seems
> like semantically this `devenv` thing is a parameter to the test, so it's a
bit
> cleaner if it is explicitly declared as such.

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:192: assert devenv == ('qunit' in files)
On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> This is clever and compact, but I'm wondering if it would be more readable to
> write:
> 
>     if devenv:
>         assert 'qunit' in files
>     else:
>         assert 'qunit' not in files
> 
> This seems to express intent better. I don't feel very strong about this one,
so
> if you don't like using 4 lines instead of one, may I suggest changing the
order
> in your version:
> 
>     assert ('qunit' in files) == devenv
> 
> This reads a bit better I think.
> 
> P.S. Another one-liner version, that seems a bit more readable to me is this:
> 
>     assert 'qunit' in files if devenv else 'qunit' not in files
> 
> I'm happy to let you choose which one you prefer ;)

In this case i'll choose your reordered approach - it follows the somehow more
natural-seeming order of "primary-thing-to-check vs thing-to-check-against"

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:208: assert not any(size not in result for size in
ICON_SIZES)
On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> Is this the same as `assert all(size in result for size in ICON_SIZES)`?
> 
> Also, what do you think about `assert set(result) >= set(ICON_SIZES)`?

The result would be the same - but all(..) is more readable, yes - and get's rid
of 2 nots. Changing it.


Comparing the sets would be even more readable, but why greater-than-equal? From
what i see / thought of, we should expect the exact same values on both sides

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:217: template = packagerChrome.createScriptPage(
On 2017/08/03 16:52:29, Vasily Kuznetsov wrote:
> Could you please reformat this as follows:
> 
>     template = packagerChrome.createScriptPage(
>         params, ...
>     )
> 
> I know this is not mentioned in the style guide but we've been trying to stick
> to this style as it's considered more readable by some and equally readable by
> others. The rule will probably make it to the style guide at some point :D

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:220: template_scripts = [elem.attrib['src'] for
elem in
On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> Not that it matters for performance, but to improve clarity, since we're only
> doing `foo in template_scripts` checks on this, maybe it should be a set
> comprehension?

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:247: assert data_json[-1] == '\n'
On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> Maybe `data_json.endswith('\n')`?

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:255: version =
packager.getBuildVersion(str(srcdir), metadata, False)
On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> Here and in other places: it's better to do it this way: `srcdir.strpath`.

Please see comment above

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:270: '='.join((k, v)) for k, v in
data.iteritems()))
On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> What do you think about
> 
>     '{}={}'.format(k, v)
> 
> instead? Seems a bit more clear to me. Also `.iteritems()` is a performance
> optimization that's not compatible with Python 3, so it's probably better to
use
> `.items()` here.

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:276: assert trans_data.get('test_{}'.format(key),
{})\
On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> Flake8 should have told you that `'test_' + key` is better in this case. This
> will also make the whole assert statement fit in 80 lines and you won't need
to
> split the line with "\" -- a practice that PEP8 frowns upon.

Acknowledged, although flake8 / tox did not inform / complain about this. Will
look into it.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:283: {k: v for k, v in MINIMUM_MANIFEST_JSON})
On 2017/08/03 16:52:30, Vasily Kuznetsov wrote:
> You can just use `dict(MINIMUM_MANIFEST_JUST)` here for the same effect.

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:292: assert
len(ger_messages['description']['message']) <= 132
On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> This is a curious assertion. What exactly are we checking here? Maybe a
comment
> would be useful?

ChromeWebStore enforces a max-length to be applied on translated strings used in
names / descriptions - but i agree on the comment-comment

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:314: for release, devenv, ext_type, key in product(
On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> Perhaps it would be better to make this a parametrization as well. Seems a bit
> cleaner and we would see individual passes / failures for each parameter
> combination in the report, which might be more useful. What do you think?

Totally agreed!

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
File tests/test_packagerGecko.py (right):

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:31: """Examplary scripts for testing
addMissingFiles"""
On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> AFAIK "examplary" nowadays is usually spelled "exemplary" and it's usually
used
> to mean something like "outstanding". I'd just say "Example scripts ..." but
> perhaps you can also check with Tom ;)

Your are right - but i like the "outstanding" part - will change it though ;)

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:56: 'description': {'message': 'Description
translated'}
On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> When you use multiline layout for lists, sets and dicts, it's better to add a
> comma to the last line as well. This allows adding more items later without
> changing unrelated lines.

As you can see in other collection-defintions, i normally do this - typo, fixing
it :)

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:70: tmp_dir.join('subs.xml').write('\n'.join((
On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> Wouldn't it be easier to just use a string literal here? If you don't like
that,
> it would be better to make the argument to `join` a list rather than a tuple.

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:100: for incomplete in [True, False]:
On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> This could also be done via parametrize perhaps.

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:110: def test_create_manifest(tmp_dir):
On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> All this code is pretty cool and clever, but it makes me wonder if there could
> be a simpler way to check the manifest. For example, could we just put an
> expected result into a file and then just load both (output and expected
result)
> with ElementTree and verify that they are identical except for some details
that
> we know will be different?

The cool- and cleverness could be a result of following your original-approach
;)
But yes, that would minimize the initialisation-boilerplate to a minimum. I will
adhere to this approach in test_packagerEdge.py as well!

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:185: for release, multicompartment in product(TR_FA,
TR_FA):
On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> Maybe better do this with parametrization?

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:202: with open('/tmp/test.xml', 'w') as fp:
On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> Why use '/tmp' instead of tmpdir fixture? Also keep in mind that you can't
> really depend on '/tmp' always being there and you having write access to it.

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:215: for x in res:
On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> Seems like you just want to compare `res` to `value` as sets here:
> 
>     assert set(res) == set(value)
> 
> However, this also seems redundant given that the assert below (line 219)
always
> runs.

I agree with the assert set() == set() part, but only relying on assert 'res ==
value' from line 219 would require those creating the data to keep all potential
elements in the exact same order (which is not guaranteed while using etree
iirc) - or am i missing something?

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:305: for all_locales, release, multicompartment in
product(
On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> Also maybe parametrization?

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29501559/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29501558/diff/29501559/tox.ini#newcode32
tox.ini:32: py.test tests --capture=sys
On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> What is the point of this capturing setup?

make_icons in packagerChrome warns the user (via stdout) about non-quare images,
which is checked by the respective test-function

tlucas

Path Set 2 * Added example manifests for edge and gecko to check against * ...

Aug. 4, 2017, 2:52 p.m. (2017-08-04 14:52:01 UTC) #7

Path Set 2

* Added example manifests for edge and gecko to check against
* implemented proper use of parametrize
* resolved pep8-issues
* refactored metadata-provisioning
* minor pythonic tweaks

https://codereview.adblockplus.org/29501558/diff/29501559/tests/conftest.py
File tests/conftest.py (right):

https://codereview.adblockplus.org/29501558/diff/29501559/tests/conftest.py#n...
tests/conftest.py:29: @pytest.fixture
On 2017/08/03 21:26:00, tlucas wrote:
> On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> > What's the purpose of this vs. just using `tmpdir`?
> 
> Iirc the tmpdir fixture would not have allowed me to have multiple other
> fixtures use the same directory when needed - i'll look into this though and
> check back with you

That actually is the reason, why i implemented 1 fixture "tmp_dir"

https://codereview.adblockplus.org/29501558/diff/29501559/tests/conftest.py#n...
tests/conftest.py:36: def chars():
On 2017/08/03 21:25:59, tlucas wrote:
> On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> > This name is quite generic. What do you think about renaming this to
something
> a
> > bit more specific, like `str500chars`, to make the fixture easier to find
and
> > also giving it a docstring?
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
File tests/test_packagerChrome.py (right):

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:18: TR_FA = [True, False]
On 2017/08/03 21:26:01, tlucas wrote:
> On 2017/08/03 16:52:29, Vasily Kuznetsov wrote:
> > Not sure having this constant is worth it. TR_FA is a bit of a confusing
name
> > and if you call it TRUE_FALSE, then it's kind of pointless to have the
> constant.
> 
> This will be gotten rid of by proper parametrization anyway: Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:20: MINIMUM_MANIFEST_JSON = [
On 2017/08/03 21:26:01, tlucas wrote:
> On 2017/08/03 16:52:29, Vasily Kuznetsov wrote:
> > Isn't this name a bit confusing given that the content of this constant is
not
> > JSON but a list? Maybe just call it MINIMUM_MANIFEST?
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:21: ('manifest_version', 2),
On 2017/08/03 21:26:00, tlucas wrote:
> On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> > For consistency with other code, it would be better to indent this list as
> > follows:
> > 
> > FOO = [
> >     'bar',
> >     'baz',
> > ]
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:40: try:
On 2017/08/03 21:26:01, tlucas wrote:
> On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> > Since this runs in tests, we probably know which branch of this try/except
> will
> > actually work. Or do we not know it?
> 
> We actually do, i just added this try/except before adding pillow to the
> tox.ini. Acknowledged

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:50: paths +=
[str(icon_dir.join('abp-{}.png'.format(size)))]
On 2017/08/03 21:26:00, tlucas wrote:
> On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> > What do you think about rewriting this as follows?
> > 
> >         img_path = icon_dir.join(...).strpath
> >         img.save(img_path)
> >         paths.append(img_path)
> > 
> > Seems like it would be a bit more readable.
> > 
> > Also, using `some_pypath.strpath` is preferable to `str(some_pythonpath)`
> since
> > it's a more specific code that uses py.path API and it's also consistent
with
> > how we're doing it in most other tests.
> 
> I agree, this would be a lot more readable.
> 
> Regarding the str-comment:
> the str(some_pypath) was inspired by the original test_packagerEdge.py content
-
> which style should i follow here?
> 
> Also: Afaik the __str__ method simply returns self.strpath for now and imho
the
> explicit conversion is more readable - but i wouldn't mind changing that (and
> maybe adjust the respective code in test_packagerEdge as well?)

Done / as discussed, not changing the str()-thing

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:96: @pytest.mark.parametrize(
On 2017/08/03 21:26:00, tlucas wrote:
> On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> > What do you think about creating 3 fixtures instead: `gecko_metadata`,
> > `chrome_metadata` and `edge_metadata`? Then the tests could depend on those
> > fixtures directly instead of having to copy these `mark.parametrize` and
> > `mark.usefixtures` everywhere.
> 
> This would also reduce the amount of readMetadata-calls. Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:162: expected = ''.join((
On 2017/08/03 21:26:02, tlucas wrote:
> On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> > You can actually write this as follows:
> > 
> >     expected = (
> >         '3495802348590234850934850234509'
> >         '4909324063490682390462820934680'
> >         ...
> >     )
> > 
> > When a bunch of string literals are written one after another like this,
> Python
> > will just concatenate them into one big constant.
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:180: extensions = ['html', 'js', 'json', 'xml']
On 2017/08/03 21:26:00, tlucas wrote:
> On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> > What do you think about replacing this with:
> > 
> >     files = ['foo.html', 'foo.js', 'foo.json', 'foo.xml']
> > 
> > Then you wouldn't need to prepend 'foo.' to each extension twice in the
> > following code and it would become simpler and more readable.
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:184: for devenv in [True, False]:
On 2017/08/03 21:26:00, tlucas wrote:
> On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> > What do you think about making devenv a parameter to the test:
> > 
> > @pytest.mark.parametrize('devenv', [True, False])
> > def test_get_package_files(srcdir, devenv):
> >     ...
> > 
> > This would convert this test into two test items that might pass or fail
> > independently thus producing better granularity of the reporting. And it
seems
> > like semantically this `devenv` thing is a parameter to the test, so it's a
> bit
> > cleaner if it is explicitly declared as such.
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:192: assert devenv == ('qunit' in files)
On 2017/08/03 21:26:01, tlucas wrote:
> On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> > This is clever and compact, but I'm wondering if it would be more readable
to
> > write:
> > 
> >     if devenv:
> >         assert 'qunit' in files
> >     else:
> >         assert 'qunit' not in files
> > 
> > This seems to express intent better. I don't feel very strong about this
one,
> so
> > if you don't like using 4 lines instead of one, may I suggest changing the
> order
> > in your version:
> > 
> >     assert ('qunit' in files) == devenv
> > 
> > This reads a bit better I think.
> > 
> > P.S. Another one-liner version, that seems a bit more readable to me is
this:
> > 
> >     assert 'qunit' in files if devenv else 'qunit' not in files
> > 
> > I'm happy to let you choose which one you prefer ;)
> 
> In this case i'll choose your reordered approach - it follows the somehow more
> natural-seeming order of "primary-thing-to-check vs thing-to-check-against" 

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:208: assert not any(size not in result for size in
ICON_SIZES)
On 2017/08/03 21:26:02, tlucas wrote:
> On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> > Is this the same as `assert all(size in result for size in ICON_SIZES)`?
> > 
> > Also, what do you think about `assert set(result) >= set(ICON_SIZES)`?
> 
> The result would be the same - but all(..) is more readable, yes - and get's
rid
> of 2 nots. Changing it.
> 
> 
> Comparing the sets would be even more readable, but why greater-than-equal?
From
> what i see / thought of, we should expect the exact same values on both sides

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:217: template = packagerChrome.createScriptPage(
On 2017/08/03 21:26:01, tlucas wrote:
> On 2017/08/03 16:52:29, Vasily Kuznetsov wrote:
> > Could you please reformat this as follows:
> > 
> >     template = packagerChrome.createScriptPage(
> >         params, ...
> >     )
> > 
> > I know this is not mentioned in the style guide but we've been trying to
stick
> > to this style as it's considered more readable by some and equally readable
by
> > others. The rule will probably make it to the style guide at some point :D
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:220: template_scripts = [elem.attrib['src'] for
elem in
On 2017/08/03 21:26:01, tlucas wrote:
> On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> > Not that it matters for performance, but to improve clarity, since we're
only
> > doing `foo in template_scripts` checks on this, maybe it should be a set
> > comprehension?
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:247: assert data_json[-1] == '\n'
On 2017/08/03 21:26:01, tlucas wrote:
> On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> > Maybe `data_json.endswith('\n')`?
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:255: version =
packager.getBuildVersion(str(srcdir), metadata, False)
On 2017/08/03 21:26:01, tlucas wrote:
> On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> > Here and in other places: it's better to do it this way: `srcdir.strpath`.
> 
> Please see comment above

As discussed

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:270: '='.join((k, v)) for k, v in
data.iteritems()))
On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> What do you think about
> 
>     '{}={}'.format(k, v)
> 
> instead? Seems a bit more clear to me. Also `.iteritems()` is a performance
> optimization that's not compatible with Python 3, so it's probably better to
use
> `.items()` here.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:276: assert trans_data.get('test_{}'.format(key),
{})\
On 2017/08/03 21:26:00, tlucas wrote:
> On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> > Flake8 should have told you that `'test_' + key` is better in this case.
This
> > will also make the whole assert statement fit in 80 lines and you won't need
> to
> > split the line with "\" -- a practice that PEP8 frowns upon.
> 
> Acknowledged, although flake8 / tox did not inform / complain about this. Will
> look into it.

Done. No idea why flake8 didn't bother though

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:283: {k: v for k, v in MINIMUM_MANIFEST_JSON})
On 2017/08/03 21:26:00, tlucas wrote:
> On 2017/08/03 16:52:30, Vasily Kuznetsov wrote:
> > You can just use `dict(MINIMUM_MANIFEST_JUST)` here for the same effect.
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:292: assert
len(ger_messages['description']['message']) <= 132
On 2017/08/03 21:26:02, tlucas wrote:
> On 2017/08/03 16:52:28, Vasily Kuznetsov wrote:
> > This is a curious assertion. What exactly are we checking here? Maybe a
> comment
> > would be useful?
> 
> ChromeWebStore enforces a max-length to be applied on translated strings used
in
> names / descriptions - but i agree on the comment-comment

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerChrome.py:314: for release, devenv, ext_type, key in product(
On 2017/08/03 21:26:01, tlucas wrote:
> On 2017/08/03 16:52:31, Vasily Kuznetsov wrote:
> > Perhaps it would be better to make this a parametrization as well. Seems a
bit
> > cleaner and we would see individual passes / failures for each parameter
> > combination in the report, which might be more useful. What do you think?
> 
> Totally agreed!

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
File tests/test_packagerGecko.py (right):

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:31: """Examplary scripts for testing
addMissingFiles"""
On 2017/08/03 21:26:02, tlucas wrote:
> On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> > AFAIK "examplary" nowadays is usually spelled "exemplary" and it's usually
> used
> > to mean something like "outstanding". I'd just say "Example scripts ..." but
> > perhaps you can also check with Tom ;)
> 
> Your are right - but i like the "outstanding" part - will change it though ;)

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:56: 'description': {'message': 'Description
translated'}
On 2017/08/03 21:26:03, tlucas wrote:
> On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> > When you use multiline layout for lists, sets and dicts, it's better to add
a
> > comma to the last line as well. This allows adding more items later without
> > changing unrelated lines.
> 
> As you can see in other collection-defintions, i normally do this - typo,
fixing
> it :)

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:70: tmp_dir.join('subs.xml').write('\n'.join((
On 2017/08/03 21:26:03, tlucas wrote:
> On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> > Wouldn't it be easier to just use a string literal here? If you don't like
> that,
> > it would be better to make the argument to `join` a list rather than a
tuple.
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:100: for incomplete in [True, False]:
On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> This could also be done via parametrize perhaps.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:110: def test_create_manifest(tmp_dir):
On 2017/08/03 21:26:02, tlucas wrote:
> On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> > All this code is pretty cool and clever, but it makes me wonder if there
could
> > be a simpler way to check the manifest. For example, could we just put an
> > expected result into a file and then just load both (output and expected
> result)
> > with ElementTree and verify that they are identical except for some details
> that
> > we know will be different?
> 
> The cool- and cleverness could be a result of following your original-approach
> ;)
> But yes, that would minimize the initialisation-boilerplate to a minimum. I
will
> adhere to this approach in test_packagerEdge.py as well!

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:185: for release, multicompartment in product(TR_FA,
TR_FA):
On 2017/08/03 21:26:02, tlucas wrote:
> On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> > Maybe better do this with parametrization?
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:202: with open('/tmp/test.xml', 'w') as fp:
On 2017/08/03 21:26:02, tlucas wrote:
> On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> > Why use '/tmp' instead of tmpdir fixture? Also keep in mind that you can't
> > really depend on '/tmp' always being there and you having write access to
it.
> 
> Acknowledged.

Done (removed).

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:215: for x in res:
On 2017/08/03 21:26:02, tlucas wrote:
> On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> > Seems like you just want to compare `res` to `value` as sets here:
> > 
> >     assert set(res) == set(value)
> > 
> > However, this also seems redundant given that the assert below (line 219)
> always
> > runs.
> 
> I agree with the assert set() == set() part, but only relying on assert 'res
==
> value' from line 219 would require those creating the data to keep all
potential
> elements in the exact same order (which is not guaranteed while using etree
> iirc) - or am i missing something?

I totally missed something. Done.

https://codereview.adblockplus.org/29501558/diff/29501559/tests/test_packager...
tests/test_packagerGecko.py:305: for all_locales, release, multicompartment in
product(
On 2017/08/03 21:26:03, tlucas wrote:
> On 2017/08/03 16:52:32, Vasily Kuznetsov wrote:
> > Also maybe parametrization?
> 
> Acknowledged.

Done.

Vasily Kuznetsov

Hi Tristan, I'm sorry that I haven't replied to the review earlier: somehow it slipped ...

Aug. 10, 2017, 7:48 p.m. (2017-08-10 19:48:29 UTC) #8

tlucas

Path Set 3 * renaming files to base_files * adding missing commas to some lines ...

Aug. 11, 2017, 12:17 p.m. (2017-08-11 12:17:10 UTC) #9

Vasily Kuznetsov

Hi Tristan, After another more careful look, I have some more comments, but in general ...

Aug. 11, 2017, 4:46 p.m. (2017-08-11 16:46:03 UTC) #10

Hi Tristan,

After another more careful look, I have some more comments, but in general it
looks pretty much done, as far as I'm concerned.

One question that didn't fit into inline comments: we don't seem to need
"tests/__init__.py" -- the tests work fine without it. Maybe we just remove it?

Cheers,
Vasily

https://codereview.adblockplus.org/29501558/diff/29505589/tests/test_packager...
File tests/test_packagerChrome.py (right):

https://codereview.adblockplus.org/29501558/diff/29505589/tests/test_packager...
tests/test_packagerChrome.py:125: if ext_type == 'chrome':
On 2017/08/11 12:17:09, tlucas wrote:
> On 2017/08/10 19:48:28, Vasily Kuznetsov wrote:
> > Instead of this you could do:
> > 
> > @pytest.mark.parametrize('ext_type,metadata', [
> >     ('chrome', chrome_metadata),
> >     ('gecko-webext', gecko_webext_metadata),
> > ])
> > 
> > this would be a bit cleaner.
> 
> I thought about that too and i definitely agree with you, but according to ->
> https://github.com/pytest-dev/pytest/issues/349
> parametrizing with a fixture is a not yet implemented proposal. I could call
the
> xxx_metadata()-functions in the decorator, but iirc that would call them for
> every permutation of the parameters, which we don't want.
> 
> if you don't mind i'll leave this as is.

Yes, you're right.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/conftest.py
File tests/conftest.py (right):

https://codereview.adblockplus.org/29501558/diff/29512631/tests/conftest.py#n...
tests/conftest.py:41: lib_dir = tmpdir.mkdir('lib')
It would be cleaner to make lib_dir a separate fixture and user it here and
elsewhere, where we need it.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
File tests/test_packagerChrome.py (right):

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:18: MINIMUM_MANIFEST = [
Would it perhaps be more elegant to make this a dictionary? Seems like the data
is more like a dictionary and we mostly seem to be using this as a dictionary.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:48: return packager.readMetadata(str(tmpdir),
'chrome')
Could we just read the metadata from the original file, without copying it, or
does it not work for some reason?

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:57: assert chrome_metadata is not None
Do you really need this assert? Seems like if this fixture depends on the other,
the file would have been copied and the metadata would have been read.

Also, if the idea above, about reading the metadata from the original files in
the tests instead of their copies in `tmpdir` work, you don't even need to
depend on `chrome_metadata` fixture here.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:83: content = """-----BEGIN RSA PRIVATE KEY-----
What do you think about putting this into a keyfile in the tests directory and
just returning that path?

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:141: expected_values = MINIMUM_MANIFEST + [
What do you think about just doing these checks directly instead of adding them
to `expected_values`?

Something like:

    gecko_app = manifest['applications']['gecko']
    assert gecko_app['id'] == '{.....}'
    assert gecko_app['strick_min_version'] == '50.0'

    if release:
        assert manifest['name'] == '__MSG_name_release__'
        assert manifest['version'] == '1.2.3'
    else:
        assert manifest['name'] == '__MSG_name__'
        assert manifest['version'] == '1.2.3.0'
        assert gecko_app['update_url'] == DEV_UPDATE_URL

I think this style is a bit easier to follow and it's not really that much more
wordy.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:197: assert publickey.encode('hex') == expected
I just had an idea, what if we just take an MD5 or SHA hash of the value and
compare it to the hash of the expected value? Then we wouldn't need to keep this
long constant in the test (only its hash), but I don't think the test would
become any less robust.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:271: json.loads(data_json)
Maybe also compare it to the original?

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
File tests/test_packagerGecko.py (right):

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerGecko.py:22: MESSAGES = '\n'.join((
Our style guide recommends that things that you iterate over be lists, so it
would be better to make this a list.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerGecko.py:219: base_files = [
Not a big deal, but a small simplification, perhaps. What if we change this line
to: `expected = [` and then the following if statement would be something like:

    if all_locales is not None:
        expected += [...stuff...]

Is that a bit easier to follow or not really?

https://codereview.adblockplus.org/29501558/diff/29512631/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29501558/diff/29512631/tox.ini#newcode32
tox.ini:32: py.test tests --capture=sys
While we're at it, let's add coverage measurement and remove the dot in
`py.test`:

    pytest tests --capture=sys --cov=buildtools --cov-report=term-missing

You will need to also add pytest-cov to the dependency list.

tlucas

Patch Set 4 Added pytest-cov, outsourced / changed big constants (RSA-key, hash), enhanced readability according ...

Aug. 14, 2017, 2:23 p.m. (2017-08-14 14:23:18 UTC) #11

Patch Set 4

Added pytest-cov, outsourced / changed big constants (RSA-key, hash), enhanced
readability according to comments.

> One question that didn't fit into inline comments: we don't seem to need
> "tests/__init__.py" -- the tests work fine without it. Maybe we just remove
it?

As discussed, this is needed to be able to import tests/tools.py.

Cheers,
Tristan

https://codereview.adblockplus.org/29501558/diff/29512631/tests/conftest.py
File tests/conftest.py (right):

https://codereview.adblockplus.org/29501558/diff/29512631/tests/conftest.py#n...
tests/conftest.py:41: lib_dir = tmpdir.mkdir('lib')
On 2017/08/11 16:46:00, Vasily Kuznetsov wrote:
> It would be cleaner to make lib_dir a separate fixture and user it here and
> elsewhere, where we need it.

Done.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
File tests/test_packagerChrome.py (right):

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:18: MINIMUM_MANIFEST = [
On 2017/08/11 16:46:02, Vasily Kuznetsov wrote:
> Would it perhaps be more elegant to make this a dictionary? Seems like the
data
> is more like a dictionary and we mostly seem to be using this as a dictionary.

Done.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:48: return packager.readMetadata(str(tmpdir),
'chrome')
On 2017/08/11 16:46:01, Vasily Kuznetsov wrote:
> Could we just read the metadata from the original file, without copying it, or
> does it not work for some reason?

It doesn't, since the underlying functions almost every time accesses paths
relative to the metadata-file,

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:57: assert chrome_metadata is not None
On 2017/08/11 16:46:02, Vasily Kuznetsov wrote:
> Do you really need this assert? Seems like if this fixture depends on the
other,
> the file would have been copied and the metadata would have been read.
> 
> Also, if the idea above, about reading the metadata from the original files in
> the tests instead of their copies in `tmpdir` work, you don't even need to
> depend on `chrome_metadata` fixture here.

As mentioned above, using the original-file won't work. But the assert is not
necessary, removed it.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:83: content = """-----BEGIN RSA PRIVATE KEY-----
On 2017/08/11 16:46:01, Vasily Kuznetsov wrote:
> What do you think about putting this into a keyfile in the tests directory and
> just returning that path?

Done.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:141: expected_values = MINIMUM_MANIFEST + [
On 2017/08/11 16:46:01, Vasily Kuznetsov wrote:
> What do you think about just doing these checks directly instead of adding
them
> to `expected_values`?
> 
> Something like:
> 
>     gecko_app = manifest['applications']['gecko']
>     assert gecko_app['id'] == '{.....}'
>     assert gecko_app['strick_min_version'] == '50.0'
> 
>     if release:
>         assert manifest['name'] == '__MSG_name_release__'
>         assert manifest['version'] == '1.2.3'
>     else:
>         assert manifest['name'] == '__MSG_name__'
>         assert manifest['version'] == '1.2.3.0'
>         assert gecko_app['update_url'] == DEV_UPDATE_URL
> 
> I think this style is a bit easier to follow and it's not really that much
more
> wordy.

Looks definitely cleaner, done.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:197: assert publickey.encode('hex') == expected
On 2017/08/11 16:46:01, Vasily Kuznetsov wrote:
> I just had an idea, what if we just take an MD5 or SHA hash of the value and
> compare it to the hash of the expected value? Then we wouldn't need to keep
this
> long constant in the test (only its hash), but I don't think the test would
> become any less robust.

Good point, i started using SHA-hashes in the other tests i currently work on -
would serve consistency too, Done.

https://codereview.adblockplus.org/29501558/diff/29512631/tests/test_packager...
tests/test_packagerChrome.py:271: json.loads(data_json)
On 2017/08/11 16:46:01, Vasily Kuznetsov wrote:
> Maybe also compare it to the original?

Done.

https://codereview.adblockplus.org/29501558/diff/29512631/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29501558/diff/29512631/tox.ini#newcode32
tox.ini:32: py.test tests --capture=sys
On 2017/08/11 16:46:02, Vasily Kuznetsov wrote:
> While we're at it, let's add coverage measurement and remove the dot in
> `py.test`:
> 
>     pytest tests --capture=sys --cov=buildtools --cov-report=term-missing
> 
> You will need to also add pytest-cov to the dependency list.

Done. But py.test is valid too - why did you suggest omitting the dot?

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chrome File tests/metadata.chrome (right): https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chrome#newcode3 tests/metadata.chrome:3: author = Eyeo GmbH "eyeo" is spelled lower-case. Alternativley, ...

Aug. 14, 2017, 3:20 p.m. (2017-08-14 15:20:13 UTC) #12

tlucas

Patch Set 5 Hi everyone. This is a rather huge change, compared to Patch Set ...

Sept. 6, 2017, 9:04 a.m. (2017-09-06 09:04:53 UTC) #13

Patch Set 5

Hi everyone.
This is a rather huge change, compared to Patch Set 4 - in case it is desired, i
will create a new review for this.

* Included coverage into the text
* Ignoring packagerGecko as it is not to be used anymore
* Refactoring tests to create builds with the actual used API (processArgs)
where possible
* Manually creating release-builds (in order to not trigger releaseAutomation)
* tried to adhere to recent comments

Please feel free to point out as much errors as possible, i could definitely use
the input.

Cheers,
Tristan

https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chrome
File tests/metadata.chrome (right):

https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chro...
tests/metadata.chrome:3: author = Eyeo GmbH
On 2017/08/14 15:20:11, Sebastian Noack wrote:
> "eyeo" is spelled lower-case. Alternativley, you might want to use an example
> company name here, but if you use the real one, please spell it correctly at
> least. Also consider updating the tests for the Edge packager.

Done.

https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chro...
tests/metadata.chrome:23: ../lib/compat.js
On 2017/08/14 15:20:12, Sebastian Noack wrote:
> Do we need all the files listed in "backgroundScripts" and "testScripts" to
> cover all code paths? The purpose of an integration test like this is not to
> duplicate the exact same scenario which we have in production, but to provide
a
> minimal example that covers as many scenarios as possible.

Done.

https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chro...
tests/metadata.chrome:27: 
On 2017/08/14 15:20:11, Sebastian Noack wrote:
> We probably should also add a [contentScripts] section and test that the
scripts
> listed there are added to the build. It seems this is missing in the existing
> tests for the Edge packager as well, hence should be added there as well.

Done, but: packagerEdge does not handle a section "contentScripts"

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23
tox.ini:23: pillow
On 2017/08/14 15:20:12, Sebastian Noack wrote:
> The README (in adblockpluschrome) lists PIL (not Pillow) as a dependency:
> https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> 
> Pillow is meant to be a compatible drop-in replacement for PIL. So in theory
> buildtools should work with both, and since in some environments only either
is
> available, it would be a good idea to ensure compatiblity with PIL and Pillow:
> 
>   [tox]
>   envlist=py27-{PIL,pillow}
> 
>   [testenv]
>   deps=
>       PIL: PIL
>       pillow: pillow
>       ...

Got rid of PIL / pillow dependency completely (by adding pre-generated images)

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chrome File tests/metadata.chrome (right): https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chrome#newcode27 tests/metadata.chrome:27: On 2017/09/06 09:04:51, tlucas wrote: > On 2017/08/14 15:20:11, ...

Sept. 6, 2017, 6 p.m. (2017-09-06 18:00:01 UTC) #14

https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chrome
File tests/metadata.chrome (right):

https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chro...
tests/metadata.chrome:27: 
On 2017/09/06 09:04:51, tlucas wrote:
> On 2017/08/14 15:20:11, Sebastian Noack wrote:
> > We probably should also add a [contentScripts] section and test that the
> scripts
> > listed there are added to the build. It seems this is missing in the
existing
> > tests for the Edge packager as well, hence should be added there as well.
> 
> Done, but: packagerEdge does not handle a section "contentScripts"

Not sure what you mean but there are definitely content scripts in the Adblock
Plus for Microsoft Edge builds. However, note that metadata.edge in
adblockpluschrome inherits most of its options from metadata.chrome:
https://hg.adblockplus.org/adblockpluschrome/file/a401d9555223/metadata.edge#l2

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23
tox.ini:23: pillow
On 2017/09/06 09:04:52, tlucas wrote:
> On 2017/08/14 15:20:12, Sebastian Noack wrote:
> > The README (in adblockpluschrome) lists PIL (not Pillow) as a dependency:
> > https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> > 
> > Pillow is meant to be a compatible drop-in replacement for PIL. So in theory
> > buildtools should work with both, and since in some environments only either
> is
> > available, it would be a good idea to ensure compatiblity with PIL and
Pillow:
> > 
> >   [tox]
> >   envlist=py27-{PIL,pillow}
> > 
> >   [testenv]
> >   deps=
> >       PIL: PIL
> >       pillow: pillow
> >       ...
> 
> Got rid of PIL / pillow dependency completely (by adding pre-generated images)

Well, the make_icons() function in packagerChrome.py is using PIL/Pillow as
well. But since this seems to be the only use of PIL/Pillow, we still have, and
getting the size of a PNG image is simple enough without an image processing
library, we might want to just do that and get rid of that dependency.

https://stackoverflow.com/questions/8032642/how-to-obtain-image-size-using-st...

Vasily Kuznetsov

Hi Tristan! I didn't get the time to look through all the code thoroughly, so ...

Sept. 7, 2017, 8:27 p.m. (2017-09-07 20:27:51 UTC) #15

Hi Tristan!

I didn't get the time to look through all the code thoroughly, so just one
comment below for now. BTW, do the tests pass for you? For me everything in
test_packagerWebExt.py fails (see below).

Cheers,
Vasily


$ tox
....yada yada yada....
collected 11 items

tests/test_packagerEdge.py .....
tests/test_packagerWebExt.py FFFFFF

....crash boom bang.....

tests/tools.py:123: in run_webext_build
    processArgs(str(srcdir), args)
build.py:607: in processArgs
    commands[command](baseDir, scriptName, opts, args, type)
build.py:55: in __call__
    return self._handler(baseDir, scriptName, opts, args, type)
build.py:206: in runBuild
    packager.createBuild(baseDir, type=type, **kwargs)
packagerChrome.py:404: in createBuild
    files['manifest.json'] = createManifest(params, files)
packagerChrome.py:91: in createManifest
    icon = makeIcons(files, icons)
packagerChrome.py:52: in makeIcons
    width, height = Image.open(StringIO(files[filename])).size

....some source code.......

E       IOError: cannot identify image file <StringIO.StringIO instance at
0x1039e8ab8>

.tox/py27/lib/python2.7/site-packages/PIL/Image.py:2519: IOError

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23
tox.ini:23: pillow
On 2017/09/06 18:00:00, Sebastian Noack wrote:
> On 2017/09/06 09:04:52, tlucas wrote:
> > On 2017/08/14 15:20:12, Sebastian Noack wrote:
> > > The README (in adblockpluschrome) lists PIL (not Pillow) as a dependency:
> > >
https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> > > 
> > > Pillow is meant to be a compatible drop-in replacement for PIL. So in
theory
> > > buildtools should work with both, and since in some environments only
either
> > is
> > > available, it would be a good idea to ensure compatiblity with PIL and
> Pillow:
> > > 
> > >   [tox]
> > >   envlist=py27-{PIL,pillow}
> > > 
> > >   [testenv]
> > >   deps=
> > >       PIL: PIL
> > >       pillow: pillow
> > >       ...
> > 
> > Got rid of PIL / pillow dependency completely (by adding pre-generated
images)
> 
> Well, the make_icons() function in packagerChrome.py is using PIL/Pillow as
> well. But since this seems to be the only use of PIL/Pillow, we still have,
and
> getting the size of a PNG image is simple enough without an image processing
> library, we might want to just do that and get rid of that dependency.
> 
>
https://stackoverflow.com/questions/8032642/how-to-obtain-image-size-using-st...

Guys! Writing your own image format parser is rather radical NIHilism, given
that Pillow library is available and is quite stable and standard. However, if
it's only about PNG, it is not too complicated indeed, so be my guests, knock
yourselves out ;)

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23 tox.ini:23: pillow On 2017/09/07 20:27:50, Vasily Kuznetsov wrote: > On ...

Sept. 7, 2017, 8:39 p.m. (2017-09-07 20:39:00 UTC) #16

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23
tox.ini:23: pillow
On 2017/09/07 20:27:50, Vasily Kuznetsov wrote:
> On 2017/09/06 18:00:00, Sebastian Noack wrote:
> > On 2017/09/06 09:04:52, tlucas wrote:
> > > On 2017/08/14 15:20:12, Sebastian Noack wrote:
> > > > The README (in adblockpluschrome) lists PIL (not Pillow) as a
dependency:
> > > >
> https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> > > > 
> > > > Pillow is meant to be a compatible drop-in replacement for PIL. So in
> theory
> > > > buildtools should work with both, and since in some environments only
> either
> > > is
> > > > available, it would be a good idea to ensure compatiblity with PIL and
> > Pillow:
> > > > 
> > > >   [tox]
> > > >   envlist=py27-{PIL,pillow}
> > > > 
> > > >   [testenv]
> > > >   deps=
> > > >       PIL: PIL
> > > >       pillow: pillow
> > > >       ...
> > > 
> > > Got rid of PIL / pillow dependency completely (by adding pre-generated
> images)
> > 
> > Well, the make_icons() function in packagerChrome.py is using PIL/Pillow as
> > well. But since this seems to be the only use of PIL/Pillow, we still have,
> and
> > getting the size of a PNG image is simple enough without an image processing
> > library, we might want to just do that and get rid of that dependency.
> > 
> >
>
https://stackoverflow.com/questions/8032642/how-to-obtain-image-size-using-st...
> 
> Guys! Writing your own image format parser is rather radical NIHilism, given
> that Pillow library is available and is quite stable and standard. However, if
> it's only about PNG, it is not too complicated indeed, so be my guests, knock
> yourselves out ;)

Well, all we need at the current point, is extracting the image size of an PNG
image. This is a trivial thing to do by parsing the relevant bytes from the PNG
header, and would avoid having us to instruct users to install either PIL or
Pillow, and testing our code with both libraries.

tlucas

Patch Set 6 * reuploaded images (upload.py can now properly handle binary files, credits to ...

Sept. 8, 2017, 10:31 a.m. (2017-09-08 10:31:28 UTC) #17

Patch Set 6

* reuploaded images (upload.py can now properly handle binary files, credits to
Vasily)


On 2017/09/07 20:27:51, Vasily Kuznetsov wrote:
> Hi Tristan!
> 
> I didn't get the time to look through all the code thoroughly, so just one
> comment below for now. BTW, do the tests pass for you? For me everything in
> test_packagerWebExt.py fails (see below).
> 
> Cheers,
> Vasily
> 
> 
> $ tox
> ....yada yada yada....
> collected 11 items
> 
> tests/test_packagerEdge.py .....
> tests/test_packagerWebExt.py FFFFFF
> 
> ....crash boom bang.....
> 
> tests/tools.py:123: in run_webext_build
>     processArgs(str(srcdir), args)
> build.py:607: in processArgs
>     commands[command](baseDir, scriptName, opts, args, type)
> build.py:55: in __call__
>     return self._handler(baseDir, scriptName, opts, args, type)
> build.py:206: in runBuild
>     packager.createBuild(baseDir, type=type, **kwargs)
> packagerChrome.py:404: in createBuild
>     files['manifest.json'] = createManifest(params, files)
> packagerChrome.py:91: in createManifest
>     icon = makeIcons(files, icons)
> packagerChrome.py:52: in makeIcons
>     width, height = Image.open(StringIO(files[filename])).size
> 
> ....some source code.......
> 
> E       IOError: cannot identify image file <StringIO.StringIO instance at
> 0x1039e8ab8>



As discussed in our IRC-journey, it works now ;)



On 2017/09/07 20:39:00, Sebastian Noack wrote:
> https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini
> File tox.ini (right):
> 
> https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23
> tox.ini:23: pillow
> On 2017/09/07 20:27:50, Vasily Kuznetsov wrote:
> > On 2017/09/06 18:00:00, Sebastian Noack wrote:
> > > On 2017/09/06 09:04:52, tlucas wrote:
> > > > On 2017/08/14 15:20:12, Sebastian Noack wrote:
> > > > > The README (in adblockpluschrome) lists PIL (not Pillow) as a
> dependency:
> > > > >
> > https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> > > > > 
> > > > > Pillow is meant to be a compatible drop-in replacement for PIL. So in
> > theory
> > > > > buildtools should work with both, and since in some environments only
> > either
> > > > is
> > > > > available, it would be a good idea to ensure compatiblity with PIL and
> > > Pillow:
> > > > > 
> > > > >   [tox]
> > > > >   envlist=py27-{PIL,pillow}
> > > > > 
> > > > >   [testenv]
> > > > >   deps=
> > > > >       PIL: PIL
> > > > >       pillow: pillow
> > > > >       ...
> > > > 
> > > > Got rid of PIL / pillow dependency completely (by adding pre-generated
> > images)
> > > 
> > > Well, the make_icons() function in packagerChrome.py is using PIL/Pillow
as
> > > well. But since this seems to be the only use of PIL/Pillow, we still
have,
> > and
> > > getting the size of a PNG image is simple enough without an image
processing
> > > library, we might want to just do that and get rid of that dependency.
> > > 
> > >
> >
>
https://stackoverflow.com/questions/8032642/how-to-obtain-image-size-using-st...
> > 
> > Guys! Writing your own image format parser is rather radical NIHilism, given
> > that Pillow library is available and is quite stable and standard. However,
if
> > it's only about PNG, it is not too complicated indeed, so be my guests,
knock
> > yourselves out ;)
> 
> Well, all we need at the current point, is extracting the image size of an PNG
> image. This is a trivial thing to do by parsing the relevant bytes from the
PNG
> header, and would avoid having us to instruct users to install either PIL or
> Pillow, and testing our code with both libraries.

I don't have a too strong opinion about this (i tend to not want to reimplement
this) - but either way, i guess it should be a separate issue, since it does not
relate to the tests

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini File tox.ini (right): https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23 tox.ini:23: pillow > On 2017/09/07 20:39:00, Sebastian Noack wrote: > ...

Sept. 8, 2017, 9:07 p.m. (2017-09-08 21:07:12 UTC) #18

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23
tox.ini:23: pillow
> On 2017/09/07 20:39:00, Sebastian Noack wrote:
> > https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini
> > File tox.ini (right):
> > 
> > https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23
> > tox.ini:23: pillow
> > On 2017/09/07 20:27:50, Vasily Kuznetsov wrote:
> > > On 2017/09/06 18:00:00, Sebastian Noack wrote:
> > > > On 2017/09/06 09:04:52, tlucas wrote:
> > > > > On 2017/08/14 15:20:12, Sebastian Noack wrote:
> > > > > > The README (in adblockpluschrome) lists PIL (not Pillow) as a
> > dependency:
> > > > > >
> > >
https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> > > > > > 
> > > > > > Pillow is meant to be a compatible drop-in replacement for PIL. So
in
> > > theory
> > > > > > buildtools should work with both, and since in some environments
only
> > > either
> > > > > is
> > > > > > available, it would be a good idea to ensure compatiblity with PIL
and
> > > > Pillow:
> > > > > > 
> > > > > >   [tox]
> > > > > >   envlist=py27-{PIL,pillow}
> > > > > > 
> > > > > >   [testenv]
> > > > > >   deps=
> > > > > >       PIL: PIL
> > > > > >       pillow: pillow
> > > > > >       ...
> > > > > 
> > > > > Got rid of PIL / pillow dependency completely (by adding pre-generated
> > > images)
> > > > 
> > > > Well, the make_icons() function in packagerChrome.py is using PIL/Pillow
> as
> > > > well. But since this seems to be the only use of PIL/Pillow, we still
> have,
> > > and
> > > > getting the size of a PNG image is simple enough without an image
> processing
> > > > library, we might want to just do that and get rid of that dependency.
> > > > 
> > > >
> > >
> >
>
https://stackoverflow.com/questions/8032642/how-to-obtain-image-size-using-st...
> > > 
> > > Guys! Writing your own image format parser is rather radical NIHilism,
given
> > > that Pillow library is available and is quite stable and standard.
However,
> if
> > > it's only about PNG, it is not too complicated indeed, so be my guests,
> knock
> > > yourselves out ;)
> > 
> > Well, all we need at the current point, is extracting the image size of an
PNG
> > image. This is a trivial thing to do by parsing the relevant bytes from the
> PNG
> > header, and would avoid having us to instruct users to install either PIL or
> > Pillow, and testing our code with both libraries.
> 
> I don't have a too strong opinion about this (i tend to not want to
reimplement
> this) - but either way, i guess it should be a separate issue, since it does
not
> relate to the tests

It is related to the tests, as it simplifies the tests. If we keep using
PIL/Pillow, my initial comment in this thread is still valid. But in order to
make sure everything works with both PIL and Pillow, we wouldn't only have to
add a couple additional lines to tox.ini, but also cause the tests to take twice
as long to run. Not to mention an additional dependency for users of buildtools.

Also note that detecting the size of a PNG image takes the same amount of code,
with PIL/Pillow than it does without:

  width, height = struct.unpack('>ii', files[filename][16:24])

  width, height = Image.open(StringIO(files[filename])).size

Anyway, if you still prefer to use PIL/Pillow, make at least sure that the code
is tested with both libraries.

tlucas

Patch Set 7: * purge PIL / Pillow dependency * Additional test for "move_files_to_extension" in ...

Sept. 11, 2017, 8:46 a.m. (2017-09-11 08:46:06 UTC) #19

Vasily Kuznetsov

Hi Tristan, I had a more thorough look through the changes and generally things make ...

Sept. 11, 2017, 12:50 p.m. (2017-09-11 12:50:09 UTC) #20

Hi Tristan,

I had a more thorough look through the changes and generally things make sense
to me.

I'm a bit concerned about maintainability of such an elaborate test suite, but
at this point it seems like we should just bite the bullet and land it. Having
test coverage for this code is a definite step forward and it should make
working with this codebase easier. If we end up discovering that adjusting the
tests to follow required changes in buildtools is too hard, we can look at
individual parts of the test suite and see how to make them more flexible.

It might make sense to add a README file to the tests directory describing how
the test suite fits together. It should probably also document the overall
approach to testing each packager and how things work at the high level.
Something along the lines of: "we construct a test setup that looks like
extension source doing x, y and z, then run the build in normal and devbuild
mode and afterwards assert a, b, and c".

I also left a few comments on the details (see below).

Cheers,
Vasily

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23
tox.ini:23: pillow
On 2017/09/08 21:07:12, Sebastian Noack wrote:
> > On 2017/09/07 20:39:00, Sebastian Noack wrote:
> > > https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini
> > > File tox.ini (right):
> > > 
> > >
https://codereview.adblockplus.org/29501558/diff/29515566/tox.ini#newcode23
> > > tox.ini:23: pillow
> > > On 2017/09/07 20:27:50, Vasily Kuznetsov wrote:
> > > > On 2017/09/06 18:00:00, Sebastian Noack wrote:
> > > > > On 2017/09/06 09:04:52, tlucas wrote:
> > > > > > On 2017/08/14 15:20:12, Sebastian Noack wrote:
> > > > > > > The README (in adblockpluschrome) lists PIL (not Pillow) as a
> > > dependency:
> > > > > > >
> > > >
> https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/README.md#l22
> > > > > > > 
> > > > > > > Pillow is meant to be a compatible drop-in replacement for PIL. So
> in
> > > > theory
> > > > > > > buildtools should work with both, and since in some environments
> only
> > > > either
> > > > > > is
> > > > > > > available, it would be a good idea to ensure compatiblity with PIL
> and
> > > > > Pillow:
> > > > > > > 
> > > > > > >   [tox]
> > > > > > >   envlist=py27-{PIL,pillow}
> > > > > > > 
> > > > > > >   [testenv]
> > > > > > >   deps=
> > > > > > >       PIL: PIL
> > > > > > >       pillow: pillow
> > > > > > >       ...
> > > > > > 
> > > > > > Got rid of PIL / pillow dependency completely (by adding
pre-generated
> > > > images)
> > > > > 
> > > > > Well, the make_icons() function in packagerChrome.py is using
PIL/Pillow
> > as
> > > > > well. But since this seems to be the only use of PIL/Pillow, we still
> > have,
> > > > and
> > > > > getting the size of a PNG image is simple enough without an image
> > processing
> > > > > library, we might want to just do that and get rid of that dependency.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://stackoverflow.com/questions/8032642/how-to-obtain-image-size-using-st...
> > > > 
> > > > Guys! Writing your own image format parser is rather radical NIHilism,
> given
> > > > that Pillow library is available and is quite stable and standard.
> However,
> > if
> > > > it's only about PNG, it is not too complicated indeed, so be my guests,
> > knock
> > > > yourselves out ;)
> > > 
> > > Well, all we need at the current point, is extracting the image size of an
> PNG
> > > image. This is a trivial thing to do by parsing the relevant bytes from
the
> > PNG
> > > header, and would avoid having us to instruct users to install either PIL
or
> > > Pillow, and testing our code with both libraries.
> > 
> > I don't have a too strong opinion about this (i tend to not want to
> reimplement
> > this) - but either way, i guess it should be a separate issue, since it does
> not
> > relate to the tests
> 
> It is related to the tests, as it simplifies the tests. If we keep using
> PIL/Pillow, my initial comment in this thread is still valid. But in order to
> make sure everything works with both PIL and Pillow, we wouldn't only have to
> add a couple additional lines to tox.ini, but also cause the tests to take
twice
> as long to run. Not to mention an additional dependency for users of
buildtools.
> 
> Also note that detecting the size of a PNG image takes the same amount of
code,
> with PIL/Pillow than it does without:
> 
>   width, height = struct.unpack('>ii', files[filename][16:24])
> 
>   width, height = Image.open(StringIO(files[filename])).size
> 
> Anyway, if you still prefer to use PIL/Pillow, make at least sure that the
code
> is tested with both libraries.

Seems like your proposal would work fine and it's simple and compact indeed, so
I don't mind. And eliminating dependency on PIL/Pillow is a good thing!

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
File tests/test_packagerEdge.py (right):

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerEdge.py:10: from buildtools.tests.tools import
run_webext_build
You can combine all the imports from `buildtools.tests.tools` like this:

    from buildtools.tests.tools import (run_webext_build, locale_files,
                                        copy_metadata, ...)

or like this:

    from buildtools.tests.tools import (
        run_webext_build,
        locale_files,
        copy_metadata,
        ...
    )

The first one is more compact and less cluttered, the second one is even more
readable -- choose your favorite :)

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerWebExt.py:13: from buildtools.tests.tools import DirContent
These imports also can be combined.

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerWebExt.py:150: from struct import unpack
Is there any specific reason to have these imports are inside of the function?

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerWebExt.py:256: def test_build_gecko_webext(dev_build_release,
tmpdir, srcdir, capsys):
This test seems very similar to the previous one apart from some file naming
differences. What do you think about merging them and using parametrization to
switch between the two versions of the file names (and other minor difference)?
Seems like this wouldn't make things much more complicated and we would
eliminate a lot of code duplication.

https://codereview.adblockplus.org/29501558/diff/29541651/tests/tools.py
File tests/tools.py (right):

https://codereview.adblockplus.org/29501558/diff/29541651/tests/tools.py#newc...
tests/tools.py:174: return set(get_leafs_string(tree_a)) ==
set(get_leafs_string(tree_b))
This function and the one below seems to be only used in assertions. It's
probably better to just move the `assert`s into the functions -- this way pytest
will produce more helpful error messages with the diffs between the things we're
comparing.

https://codereview.adblockplus.org/29501558/diff/29541651/tests/tools.py#newc...
tests/tools.py:177: def compare_json(a, b):
It seems like this function essentially compares two JSON strings ignoring the
order of items in lists. I think it would make sense to give it a docstring that
explains how this comparison differs from just `a == b` (and perhaps why it's
needed).

Also, I wonder if this is actually the best way to do this. What do you think
about traversing the deserialized data converting lists to sets and then just
comparing the results. Something like:

    def lst2set(obj):
        if isinstance(obj, list):
            return {lst2set(item) for item in obj}
        if isinstance(obj, dict):
            return {k, lst2set(v) for k, v in obj.items()}
        return obj

tlucas

Hey Vasily, thanks for your comments! On 2017/09/11 12:50:09, Vasily Kuznetsov wrote: > Hi Tristan, ...

Sept. 12, 2017, 11:32 a.m. (2017-09-12 11:32:13 UTC) #21

Hey Vasily,

thanks for your comments!

On 2017/09/11 12:50:09, Vasily Kuznetsov wrote:
> Hi Tristan,
> 
> I had a more thorough look through the changes and generally things make sense
> to me.
> 
> I'm a bit concerned about maintainability of such an elaborate test suite, but
> at this point it seems like we should just bite the bullet and land it. Having
> test coverage for this code is a definite step forward and it should make
> working with this codebase easier. If we end up discovering that adjusting the
> tests to follow required changes in buildtools is too hard, we can look at
> individual parts of the test suite and see how to make them more flexible.

I agree that this is somewhat hard to unwind, on the other hand thinking about a
solution which satisfies all the requirements currently involved in this project
AND at the same time has a nicer flow would be even harder i guess (and imho -
but i'm the author - it should not be too hard to adjust any test)

> It might make sense to add a README file to the tests directory describing how
> the test suite fits together. It should probably also document the overall
> approach to testing each packager and how things work at the high level.
> Something along the lines of: "we construct a test setup that looks like
> extension source doing x, y and z, then run the build in normal and devbuild
> mode and afterwards assert a, b, and c".

This is a nice idea, i will create one!

Also, please see the discussion below.

Best,
Tristan

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
File tests/test_packagerEdge.py (right):

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerEdge.py:10: from buildtools.tests.tools import
run_webext_build
On 2017/09/11 12:50:06, Vasily Kuznetsov wrote:
> You can combine all the imports from `buildtools.tests.tools` like this:
> 
>     from buildtools.tests.tools import (run_webext_build, locale_files,
>                                         copy_metadata, ...)
> 
> or like this:
> 
>     from buildtools.tests.tools import (
>         run_webext_build,
>         locale_files,
>         copy_metadata,
>         ...
>     )
> 
> The first one is more compact and less cluttered, the second one is even more
> readable -- choose your favorite :)

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerWebExt.py:13: from buildtools.tests.tools import DirContent
On 2017/09/11 12:50:07, Vasily Kuznetsov wrote:
> These imports also can be combined.

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerWebExt.py:150: from struct import unpack
On 2017/09/11 12:50:07, Vasily Kuznetsov wrote:
> Is there any specific reason to have these imports are inside of the function?

Not at all - merely a result of a speedy adjustment - will change :)

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerWebExt.py:256: def test_build_gecko_webext(dev_build_release,
tmpdir, srcdir, capsys):
On 2017/09/11 12:50:07, Vasily Kuznetsov wrote:
> This test seems very similar to the previous one apart from some file naming
> differences. What do you think about merging them and using parametrization to
> switch between the two versions of the file names (and other minor
difference)?
> Seems like this wouldn't make things much more complicated and we would
> eliminate a lot of code duplication.

I agree - will do (also i fear the complexity of the resulting test function
might be a bit over the top)

https://codereview.adblockplus.org/29501558/diff/29541651/tests/tools.py
File tests/tools.py (right):

https://codereview.adblockplus.org/29501558/diff/29541651/tests/tools.py#newc...
tests/tools.py:174: return set(get_leafs_string(tree_a)) ==
set(get_leafs_string(tree_b))
On 2017/09/11 12:50:08, Vasily Kuznetsov wrote:
> This function and the one below seems to be only used in assertions. It's
> probably better to just move the `assert`s into the functions -- this way
pytest
> will produce more helpful error messages with the diffs between the things
we're
> comparing.

Acknowledged.

https://codereview.adblockplus.org/29501558/diff/29541651/tests/tools.py#newc...
tests/tools.py:177: def compare_json(a, b):
On 2017/09/11 12:50:08, Vasily Kuznetsov wrote:
> It seems like this function essentially compares two JSON strings ignoring the
> order of items in lists. I think it would make sense to give it a docstring
that
> explains how this comparison differs from just `a == b` (and perhaps why it's
> needed).
> 
> Also, I wonder if this is actually the best way to do this. What do you think
> about traversing the deserialized data converting lists to sets and then just
> comparing the results. Something like:
> 
>     def lst2set(obj):
>         if isinstance(obj, list):
>             return {lst2set(item) for item in obj}
>         if isinstance(obj, dict):
>             return {k, lst2set(v) for k, v in obj.items()}
>         return obj

I'm not sure if it would be a good idea to just ignore a list containing equal
items, e.g. [1,2,2,3] - on the other hand that would apply to a possibly desired
order too - what do you think about this?

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29541651/tests/tools.py File tests/tools.py (right): https://codereview.adblockplus.org/29501558/diff/29541651/tests/tools.py#newcode177 tests/tools.py:177: def compare_json(a, b): On 2017/09/12 11:32:11, tlucas wrote: > ...

Sept. 12, 2017, 9:55 p.m. (2017-09-12 21:55:27 UTC) #22

tlucas

Patch Set 8 Crucial: Please note that this new patch set is implemented on top ...

Sept. 13, 2017, 1:43 p.m. (2017-09-13 13:43:27 UTC) #23

Patch Set 8

Crucial: Please note that this new patch set is implemented on top of
https://issues.adblockplus.org/ticket/5668

 * Added a README to tests/, adjusted /README.md to refence the new README
 * restructured some imports
 * Adding parametrization for buildnumbers
 * Combined packager-tests for chrome and firefox
 * added additional manifests (for all valid permutations of buildtype /
buildnumber)
 * Refactored comparison of XML / json:
   In order to preserve ordering in JSON i now use the order-safe json.dumps(..,
sort_keys=True) version,
   in combination with difflibs unified_diff(), which produces an even more sane
and informative output compared
   to what assert set() == set() would yield.

   The unified_diff approach is now also used in XML comparison, also to provide
the most informative output possible.

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
File tests/test_packagerEdge.py (right):

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerEdge.py:10: from buildtools.tests.tools import
run_webext_build
On 2017/09/12 11:32:10, tlucas wrote:
> On 2017/09/11 12:50:06, Vasily Kuznetsov wrote:
> > You can combine all the imports from `buildtools.tests.tools` like this:
> > 
> >     from buildtools.tests.tools import (run_webext_build, locale_files,
> >                                         copy_metadata, ...)
> > 
> > or like this:
> > 
> >     from buildtools.tests.tools import (
> >         run_webext_build,
> >         locale_files,
> >         copy_metadata,
> >         ...
> >     )
> > 
> > The first one is more compact and less cluttered, the second one is even
more
> > readable -- choose your favorite :)
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerWebExt.py:13: from buildtools.tests.tools import DirContent
On 2017/09/12 11:32:10, tlucas wrote:
> On 2017/09/11 12:50:07, Vasily Kuznetsov wrote:
> > These imports also can be combined.
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerWebExt.py:150: from struct import unpack
On 2017/09/12 11:32:11, tlucas wrote:
> On 2017/09/11 12:50:07, Vasily Kuznetsov wrote:
> > Is there any specific reason to have these imports are inside of the
function?
> 
> Not at all - merely a result of a speedy adjustment - will change :)

Done.

https://codereview.adblockplus.org/29501558/diff/29541651/tests/test_packager...
tests/test_packagerWebExt.py:256: def test_build_gecko_webext(dev_build_release,
tmpdir, srcdir, capsys):
On 2017/09/12 11:32:11, tlucas wrote:
> On 2017/09/11 12:50:07, Vasily Kuznetsov wrote:
> > This test seems very similar to the previous one apart from some file naming
> > differences. What do you think about merging them and using parametrization
to
> > switch between the two versions of the file names (and other minor
> difference)?
> > Seems like this wouldn't make things much more complicated and we would
> > eliminate a lot of code duplication.
> 
> I agree - will do (also i fear the complexity of the resulting test function
> might be a bit over the top)

Done.

https://codereview.adblockplus.org/29501558/diff/29541651/tests/tools.py
File tests/tools.py (right):

https://codereview.adblockplus.org/29501558/diff/29541651/tests/tools.py#newc...
tests/tools.py:177: def compare_json(a, b):
On 2017/09/12 21:55:26, Sebastian Noack wrote:
> On 2017/09/12 11:32:11, tlucas wrote:
> > On 2017/09/11 12:50:08, Vasily Kuznetsov wrote:
> > > It seems like this function essentially compares two JSON strings ignoring
> the
> > > order of items in lists. I think it would make sense to give it a
docstring
> > that
> > > explains how this comparison differs from just `a == b` (and perhaps why
> it's
> > > needed).
> > > 
> > > Also, I wonder if this is actually the best way to do this. What do you
> think
> > > about traversing the deserialized data converting lists to sets and then
> just
> > > comparing the results. Something like:
> > > 
> > >     def lst2set(obj):
> > >         if isinstance(obj, list):
> > >             return {lst2set(item) for item in obj}
> > >         if isinstance(obj, dict):
> > >             return {k, lst2set(v) for k, v in obj.items()}
> > >         return obj
> > 
> > I'm not sure if it would be a good idea to just ignore a list containing
equal
> > items, e.g. [1,2,2,3] - on the other hand that would apply to a possibly
> desired
> > order too - what do you think about this?
> 
> I agree, turning lists into sets when comparing the content of manifest.json
> will potentially hide bugs. While for some values (e.g. permissions) the order
> doesn't matter, for others like list of scripts that are executed in that
order
> the order matters. Also I cannot think of any scenario for any legit duplicate
> in a list in manifest.json.

Agreed - i came up with a different solution, please see in the patch set's
mail.

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29543665/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29501558/diff/29543665/packagerChrome.py#newcode48 packagerChrome.py:48: width, height = struct.unpack('>ii', files[filename][16:24]) I wonder whether it ...

Sept. 13, 2017, 7:18 p.m. (2017-09-13 19:18:30 UTC) #24

tlucas

Patcht Set 9 Important: Please note that this new patch set is implemented on top ...

Sept. 14, 2017, 9:39 a.m. (2017-09-14 09:39:55 UTC) #25

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29543665/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29501558/diff/29543665/packagerChrome.py#newcode48 packagerChrome.py:48: width, height = struct.unpack('>ii', files[filename][16:24]) On 2017/09/14 09:39:53, tlucas ...

Sept. 14, 2017, 4:52 p.m. (2017-09-14 16:52:00 UTC) #26

tlucas

Patch Set 10 * Additionally asserting if input file is PNG in makeIcons On 2017/09/14 ...

Sept. 19, 2017, 10:04 a.m. (2017-09-19 10:04:07 UTC) #27

Vasily Kuznetsov

Hi Tristan, Sorry for the radio-silence for a while, I was on vacation. Overall, this ...

Sept. 19, 2017, 5:52 p.m. (2017-09-19 17:52:28 UTC) #28

Hi Tristan,

Sorry for the radio-silence for a while, I was on vacation. Overall, this looks
pretty good and I really like the README of the tests. I only have minor inline
comments (see below) and one more point, which you probably shouldn't do
anything about in this review.

All those files in the `expecteddata` directory are starting to become a bit
unwieldy. Imagine updating them if we change the format of manifest files, for
example. I would say it's ok for now, but we should probably look into
converting this to an approval testing approach (see
http://coding-is-like-cooking.info/2013/09/approval-testing/) or something other
more automated way of generating all these expected results.

Cheers,
Vasily

https://codereview.adblockplus.org/29501558/diff/29549678/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29501558/diff/29549678/packagerChrome.py#n...
packagerChrome.py:50: assert header == png_tuple, '{} is no valid
PNG.'.format(filename)
Python's `assert` is not a reliable method to test for actually possible
erroneous conditions because it's removed if the code is compiled with
optimization [1]. It's only safe to rely on asserts during tests (since you know
that the tests won't run with optimizations) and my understanding is that for
real errors you should use a `raise` statement instead and raise a real
exception (not `AssertionError`).

This check is for a rather rare condition, but it's still an error that might
occur and that we (as developers of this script) have not control over. I think
we should use a `raise` here.

[1]:https://docs.python.org/2/reference/simple_stmts.html#grammar-token-assert_stmt

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md
File tests/README.md (right):

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md#new...
tests/README.md:58: `tox`
You can use a 4 space indent here instead, and I would also suggest adding a
shell prompt before "tox" so it's clear that we run it from shell prompt.
Something like this:

    $ tox

https://codereview.adblockplus.org/29501558/diff/29549678/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29549678/tests/test_packager...
tests/test_packagerWebExt.py:213: 'gecko-webext':
'adblockplusfirefox-1.2.3{}.{}',
I would probably just use the structure of `extensions` nested dict here instead
and inline the extensions. Then the `extensions` dict could be eliminated
together with some code.

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29549678/packagerChrome.py File packagerChrome.py (right): https://codereview.adblockplus.org/29501558/diff/29549678/packagerChrome.py#newcode49 packagerChrome.py:49: header = struct.unpack('>B3s2scc', files[filename][0:8]) There is no reason to ...

Sept. 20, 2017, 12:58 a.m. (2017-09-20 00:58:05 UTC) #29

https://codereview.adblockplus.org/29501558/diff/29549678/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29501558/diff/29549678/packagerChrome.py#n...
packagerChrome.py:49: header = struct.unpack('>B3s2scc', files[filename][0:8])
There is no reason to split the magic number into separate parts.

Also if there are less bytes in the file than to be unpacked, this will cause
an exception that is less descriptive than the one you would raise otherwise.

So how about this?

  try:
    magic, width, height = struct.unpack_from('>8s8xii', files[filename])
  except struct.error:
    magic = None
  if magic != '\x89PNG\r\n\x1a\n':
    raise Exception(...)

https://codereview.adblockplus.org/29501558/diff/29549678/packagerChrome.py#n...
packagerChrome.py:50: assert header == png_tuple, '{} is no valid
PNG.'.format(filename)
On 2017/09/19 17:52:26, Vasily Kuznetsov wrote:
> Python's `assert` is not a reliable method to test for actually possible
> erroneous conditions because it's removed if the code is compiled with
> optimization [1]. It's only safe to rely on asserts during tests (since you
know
> that the tests won't run with optimizations) and my understanding is that for
> real errors you should use a `raise` statement instead and raise a real
> exception (not `AssertionError`).
> 
> This check is for a rather rare condition, but it's still an error that might
> occur and that we (as developers of this script) have not control over. I
think
> we should use a `raise` here.
> 
>
[1]:https://docs.python.org/2/reference/simple_stmts.html#grammar-token-assert_stmt

I'd like to add that the general idea of programming with assertions (not only
in Python) is to test your code assumptions in order to make debugging easier.
Asserts are meant to detect programming mistakes, not to signal runtime errors
that might occur if the code is behaving as expected. That Python removes
assertions from optimized bytecode, and that you mostly find assertions in test
suites, is just the logical consequence.

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md
File tests/README.md (right):

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md#new...
tests/README.md:58: `tox`
On 2017/09/19 17:52:26, Vasily Kuznetsov wrote:
> You can use a 4 space indent here instead, and I would also suggest adding a
> shell prompt before "tox" so it's clear that we run it from shell prompt.
> Something like this:
> 
>     $ tox

Actually, since the whole paragraph consists of code, you should rather use
triple backticks and no indentation at all:

```
$ tox
```

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md#new...
tests/README.md:60: in the buildtools' rootfolder.
In English, you can not make up new words by putting two words together. So it
is "root folder", not "rootfolder".

https://codereview.adblockplus.org/29501558/diff/29549678/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29549678/tests/test_packager...
tests/test_packagerWebExt.py:205: devenv = dev_build_release == 'devenv'
There are actually 3 scenarios:

1. build.py -t {chrome|gecko-webext|edge} build -r
2. build.py -t {chrome|gecko-webext|edge} build -b <revision>
3. build.py -t {chrome|gecko-webext|edge} devenv

#1 generates release builds, and #3 is for the local development environment,
you seem to handle only these two scenarios.

However, #2 is what sitescripts is invoking, in order to generate our
development builds, this scenario doesn't seem to be handled.

tlucas

Patch Set 11 * Removing unsafe 'assert', raising a TypeError instead * Adjusting content of ...

Sept. 20, 2017, 8:52 a.m. (2017-09-20 08:52:54 UTC) #30

Patch Set 11

* Removing unsafe 'assert', raising a TypeError instead
* Adjusting content of tests/README.md according to comments
* Removed unnecessary dict from text_packagerWebExt

Hey guys, thanks for the review.
I answered to Sebastian's comment on the different cases, please see below.

Best,
Tristan

https://codereview.adblockplus.org/29501558/diff/29549678/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29501558/diff/29549678/packagerChrome.py#n...
packagerChrome.py:49: header = struct.unpack('>B3s2scc', files[filename][0:8])
On 2017/09/20 00:58:00, Sebastian Noack wrote:
> There is no reason to split the magic number into separate parts.
> 
> Also if there are less bytes in the file than to be unpacked, this will cause
> an exception that is less descriptive than the one you would raise otherwise.
> 
> So how about this?
> 
>   try:
>     magic, width, height = struct.unpack_from('>8s8xii', files[filename])
>   except struct.error:
>     magic = None
>   if magic != '\x89PNG\r\n\x1a\n':
>     raise Exception(...)

Done.

https://codereview.adblockplus.org/29501558/diff/29549678/packagerChrome.py#n...
packagerChrome.py:50: assert header == png_tuple, '{} is no valid
PNG.'.format(filename)
On 2017/09/20 00:58:00, Sebastian Noack wrote:
> On 2017/09/19 17:52:26, Vasily Kuznetsov wrote:
> > Python's `assert` is not a reliable method to test for actually possible
> > erroneous conditions because it's removed if the code is compiled with
> > optimization [1]. It's only safe to rely on asserts during tests (since you
> know
> > that the tests won't run with optimizations) and my understanding is that
for
> > real errors you should use a `raise` statement instead and raise a real
> > exception (not `AssertionError`).
> > 
> > This check is for a rather rare condition, but it's still an error that
might
> > occur and that we (as developers of this script) have not control over. I
> think
> > we should use a `raise` here.
> > 
> >
>
[1]:https://docs.python.org/2/reference/simple_stmts.html#grammar-token-assert_stmt
> 
> I'd like to add that the general idea of programming with assertions (not only
> in Python) is to test your code assumptions in order to make debugging easier.
> Asserts are meant to detect programming mistakes, not to signal runtime errors
> that might occur if the code is behaving as expected. That Python removes
> assertions from optimized bytecode, and that you mostly find assertions in
test
> suites, is just the logical consequence.

Thanks for the insight - restructured this code along with the comment above.

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md
File tests/README.md (right):

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md#new...
tests/README.md:58: `tox`
On 2017/09/20 00:58:01, Sebastian Noack wrote:
> On 2017/09/19 17:52:26, Vasily Kuznetsov wrote:
> > You can use a 4 space indent here instead, and I would also suggest adding a
> > shell prompt before "tox" so it's clear that we run it from shell prompt.
> > Something like this:
> > 
> >     $ tox
> 
> Actually, since the whole paragraph consists of code, you should rather use
> triple backticks and no indentation at all:
> 
> ```
> $ tox
> ```

Done.

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md#new...
tests/README.md:60: in the buildtools' rootfolder.
On 2017/09/20 00:58:01, Sebastian Noack wrote:
> In English, you can not make up new words by putting two words together. So it
> is "root folder", not "rootfolder".

Done.

https://codereview.adblockplus.org/29501558/diff/29549678/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29549678/tests/test_packager...
tests/test_packagerWebExt.py:205: devenv = dev_build_release == 'devenv'
On 2017/09/20 00:58:02, Sebastian Noack wrote:
> There are actually 3 scenarios:
> 
> 1. build.py -t {chrome|gecko-webext|edge} build -r
> 2. build.py -t {chrome|gecko-webext|edge} build -b <revision>
> 3. build.py -t {chrome|gecko-webext|edge} devenv
> 
> #1 generates release builds, and #3 is for the local development environment,
> you seem to handle only these two scenarios.
> 
> However, #2 is what sitescripts is invoking, in order to generate our
> development builds, this scenario doesn't seem to be handled.

You are correct about #1 and #3 - however, there is no for devenv for edge (yet,
see #4720).

Regarding #2: the run_webext_build (tests/tools.py:110) appends '-b <num>' to
the arguments interpreted by processArgs from build.py, in case buildnum is set
(which is true for every second parameter combination in this test, e.g.
'dev_build_release,buildnum' = ('build', '1337')).

Effectively, a './build.py -t <platform> build -b 1337' is indeed run for all
three platforms - chrome, gecko-webext and edge. Is this what you meant or am i
missing something?

https://codereview.adblockplus.org/29501558/diff/29549678/tests/test_packager...
tests/test_packagerWebExt.py:213: 'gecko-webext':
'adblockplusfirefox-1.2.3{}.{}',
On 2017/09/19 17:52:27, Vasily Kuznetsov wrote:
> I would probably just use the structure of `extensions` nested dict here
instead
> and inline the extensions. Then the `extensions` dict could be eliminated
> together with some code.

Done.

Vasily Kuznetsov

Hi Tristan, LGTM, and I have one question to Sebastian (below). Cheers, Vasily https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md File ...

Sept. 20, 2017, 2:53 p.m. (2017-09-20 14:53:05 UTC) #31

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md File tests/README.md (right): https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md#newcode58 tests/README.md:58: `tox` On 2017/09/20 14:53:04, Vasily Kuznetsov wrote: > On ...

Sept. 20, 2017, 9:36 p.m. (2017-09-20 21:36:14 UTC) #32

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md
File tests/README.md (right):

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md#new...
tests/README.md:58: `tox`
On 2017/09/20 14:53:04, Vasily Kuznetsov wrote:
> On 2017/09/20 00:58:01, Sebastian Noack wrote:
> > On 2017/09/19 17:52:26, Vasily Kuznetsov wrote:
> > > You can use a 4 space indent here instead, and I would also suggest adding
a
> > > shell prompt before "tox" so it's clear that we run it from shell prompt.
> > > Something like this:
> > > 
> > >     $ tox
> > 
> > Actually, since the whole paragraph consists of code, you should rather use
> > triple backticks and no indentation at all:
> > 
> > ```
> > $ tox
> > ```
> 
> Seems like it's more cluttered and harder to read source code that produces
the
> same output. Why do you think this is better than just a simple code block?

The output is usually not the same (it might depend on the markdown renderer
though). Single backticks usually (e.g. on GitHub) don't render as code block,
but render a code snippet, and are meant for code inside a text paragraph.

https://codereview.adblockplus.org/29501558/diff/29549678/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29549678/tests/test_packager...
tests/test_packagerWebExt.py:205: devenv = dev_build_release == 'devenv'
On 2017/09/20 08:52:51, tlucas wrote:
> On 2017/09/20 00:58:02, Sebastian Noack wrote:
> > There are actually 3 scenarios:
> > 
> > 1. build.py -t {chrome|gecko-webext|edge} build -r
> > 2. build.py -t {chrome|gecko-webext|edge} build -b <revision>
> > 3. build.py -t {chrome|gecko-webext|edge} devenv
> > 
> > #1 generates release builds, and #3 is for the local development
environment,
> > you seem to handle only these two scenarios.
> > 
> > However, #2 is what sitescripts is invoking, in order to generate our
> > development builds, this scenario doesn't seem to be handled.
> 
> You are correct about #1 and #3 - however, there is no for devenv for edge
(yet,
> see #4720).
> 
> Regarding #2: the run_webext_build (tests/tools.py:110) appends '-b <num>' to
> the arguments interpreted by processArgs from build.py, in case buildnum is
set
> (which is true for every second parameter combination in this test, e.g.
> 'dev_build_release,buildnum' = ('build', '1337')).
> 
> Effectively, a './build.py -t <platform> build -b 1337' is indeed run for all
> three platforms - chrome, gecko-webext and edge. Is this what you meant or am
i
> missing something?

Sorry, I misinterpreted the logic here. If I understand it correctly now, these
are the command lines you are currently testing, here:

build.py -t chrome build
build.py -t chrome build -b 1337
build.py -t chrome devenv
build.py -t chrome devenv -b 1337
build.py -t chrome release -k chrome_rsa.pem
build.py -t chrome release -k chrome_rsa.pem -b 1337
build.py -t gecko-webext build
build.py -t gecko-webext build -b 1337
build.py -t gecko-webext devenv
build.py -t gecko-webext devenv -b 1337
build.py -t gecko-webext release
build.py -t gecko-webext release -b 1337

This made me wonder how the tests even pass, since the -b option is only
supported by the "build" command, and the "release" command doesn't support the
"gecko-webext" target at all. It turned out that you are actually not calling
the "release" command, which makes sense as we certainly don't want to have the
tests perform parts of the actual release process online, which would fail
anyway if you don't have push access to the downloads repository. At some point
we probably would want to have tests for the release automation though,
potentially involving some monkey patching, but this is out of scope here.
However, it doesn't make much sense either to call the internal API generating
the release build instead (in a quite obfuscated way). Instead you should rather
run "build -r" which does exactly that, i.e. generating the release build.

Furthermore, testing the "build" command with neither the -b nor the -r option
seems redundant, as it is essentially the same as "build -b 1337", just that the
revision for the devbuild is extracted from the repository. This logic, however,
is already covered by the "devenv" command, and more importantly won't work in
the test environment anyway (falling back to 0), since the directory you build
the test extension out of isn't a Mercurial or Git repository. Not to mention
that his is a rather theoretical use case, that we don't have in practice.

So that leaves us with following command lines, that should be tested instead,
for the "chrome" and "gecko-webext" targets:

build.py -t chrome build -r -k chrome_rsa.pem
build.py -t chrome build -b 1337
build.py -t chrome devenv
build.py -t gecko-webext build -r
build.py -t gecko-webext build -b 1337
build.py -t gecko-webext devenv

Respectively, following command lines need to be tested for the "edge" target:

build.py -t edge build -r
build.py -t edge build -b 1337
build.py -t edge devenv

Yes, the "devenv" command doesn't support the "edge" target at the moment. But
this is matter to change. So feel free to mark this test as xfail, for now.

https://codereview.adblockplus.org/29501558/diff/29550602/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29501558/diff/29550602/packagerChrome.py#n...
packagerChrome.py:48: magic, width, height = struct.unpack('>8s8xii',
files[filename][:24])
This is different from my suggestion:

1. If you use struct.unpack_from (instead of struct.unpack), trailing bytes are
ignored, so that you don't have to redundantly hard-code the length of the data
strucutre we are going to unpack.

2. You don't catch struct.error. As explained in my previous comment, this will
cause an inconsistent (and less meaningful) exception to be raised if there are
less than 24 bytes of data.

https://codereview.adblockplus.org/29501558/diff/29550602/packagerChrome.py#n...
packagerChrome.py:50: raise TypeError('{} is no valid PNG.'.format(filename))
Nit; from https://adblockplus.org/coding-style#python:

  Use the + operator when concatenating exactly two strings, ...

https://codereview.adblockplus.org/29501558/diff/29550602/tests/tools.py
File tests/tools.py (right):

https://codereview.adblockplus.org/29501558/diff/29550602/tests/tools.py#newc...
tests/tools.py:125: if not build_type == 'release':
Nit: `a != b` instead of `not a == b`

Is flake8 checking this file?

Vasily Kuznetsov

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md File tests/README.md (right): https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md#newcode58 tests/README.md:58: `tox` On 2017/09/20 21:36:09, Sebastian Noack wrote: > On ...

Sept. 21, 2017, 9:29 a.m. (2017-09-21 09:29:03 UTC) #33

tlucas

Patch Set 12 * Called "./build.py ... build -r" instead of "./build.py ... release" * ...

Sept. 21, 2017, 11:35 a.m. (2017-09-21 11:35:00 UTC) #34

Patch Set 12

 * Called "./build.py ... build -r" instead of "./build.py ... release"
 * Simplified parametrization according to less combinations
 * Removed redundant example manifests according to less combinations
 * Added xfailing devenv paramter for edge
 * Correctly using struct.unpack_from now

On 2017/09/20 21:36:14, Sebastian Noack wrote:
> On 2017/09/20 08:52:51, tlucas wrote:
> > On 2017/09/20 00:58:02, Sebastian Noack wrote:
> > > There are actually 3 scenarios:
> > > 
> > > 1. build.py -t {chrome|gecko-webext|edge} build -r
> > > 2. build.py -t {chrome|gecko-webext|edge} build -b <revision>
> > > 3. build.py -t {chrome|gecko-webext|edge} devenv
> > > 
> > > #1 generates release builds, and #3 is for the local development
> environment,
> > > you seem to handle only these two scenarios.
> > > 
> > > However, #2 is what sitescripts is invoking, in order to generate our
> > > development builds, this scenario doesn't seem to be handled.
> > 
> > You are correct about #1 and #3 - however, there is no for devenv for edge
> (yet,
> > see #4720).
> > 
> > Regarding #2: the run_webext_build (tests/tools.py:110) appends '-b <num>'
to
> > the arguments interpreted by processArgs from build.py, in case buildnum is
> set
> > (which is true for every second parameter combination in this test, e.g.
> > 'dev_build_release,buildnum' = ('build', '1337')).
> > 
> > Effectively, a './build.py -t <platform> build -b 1337' is indeed run for
all
> > three platforms - chrome, gecko-webext and edge. Is this what you meant or
am
> i
> > missing something?
> 
> Sorry, I misinterpreted the logic here. If I understand it correctly now,
these
> are the command lines you are currently testing, here:
> 
> build.py -t chrome build
> build.py -t chrome build -b 1337
> build.py -t chrome devenv
> build.py -t chrome devenv -b 1337
> build.py -t chrome release -k chrome_rsa.pem
> build.py -t chrome release -k chrome_rsa.pem -b 1337
> build.py -t gecko-webext build
> build.py -t gecko-webext build -b 1337
> build.py -t gecko-webext devenv
> build.py -t gecko-webext devenv -b 1337
> build.py -t gecko-webext release
> build.py -t gecko-webext release -b 1337
> 
> This made me wonder how the tests even pass, since the -b option is only
> supported by the "build" command, and the "release" command doesn't support
the
> "gecko-webext" target at all. It turned out that you are actually not calling
> the "release" command, which makes sense as we certainly don't want to have
the
> tests perform parts of the actual release process online, which would fail
> anyway if you don't have push access to the downloads repository. At some
point
> we probably would want to have tests for the release automation though,
> potentially involving some monkey patching, but this is out of scope here.
> However, it doesn't make much sense either to call the internal API generating
> the release build instead (in a quite obfuscated way). Instead you should
rather
> run "build -r" which does exactly that, i.e. generating the release build.
> 
> Furthermore, testing the "build" command with neither the -b nor the -r option
> seems redundant, as it is essentially the same as "build -b 1337", just that
the
> revision for the devbuild is extracted from the repository. This logic,
however,
> is already covered by the "devenv" command, and more importantly won't work in
> the test environment anyway (falling back to 0), since the directory you build
> the test extension out of isn't a Mercurial or Git repository. Not to mention
> that his is a rather theoretical use case, that we don't have in practice.
> 
> So that leaves us with following command lines, that should be tested instead,
> for the "chrome" and "gecko-webext" targets:
> 
> build.py -t chrome build -r -k chrome_rsa.pem
> build.py -t chrome build -b 1337
> build.py -t chrome devenv
> build.py -t gecko-webext build -r
> build.py -t gecko-webext build -b 1337
> build.py -t gecko-webext devenv
> 

Done.

> Respectively, following command lines need to be tested for the "edge" target:
> 
> build.py -t edge build -r
> build.py -t edge build -b 1337
> build.py -t edge devenv
> 
> Yes, the "devenv" command doesn't support the "edge" target at the moment. But
> this is matter to change. So feel free to mark this test as xfail, for now.

Done.

https://codereview.adblockplus.org/29501558/diff/29550602/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29501558/diff/29550602/packagerChrome.py#n...
packagerChrome.py:48: magic, width, height = struct.unpack('>8s8xii',
files[filename][:24])
On 2017/09/20 21:36:10, Sebastian Noack wrote:
> This is different from my suggestion:
> 
> 1. If you use struct.unpack_from (instead of struct.unpack), trailing bytes
are
> ignored, so that you don't have to redundantly hard-code the length of the
data
> strucutre we are going to unpack.
> 
> 2. You don't catch struct.error. As explained in my previous comment, this
will
> cause an inconsistent (and less meaningful) exception to be raised if there
are
> less than 24 bytes of data.

Sorry, i somehow overlooked this part from your comment. Done.

https://codereview.adblockplus.org/29501558/diff/29550602/packagerChrome.py#n...
packagerChrome.py:50: raise TypeError('{} is no valid PNG.'.format(filename))
On 2017/09/20 21:36:10, Sebastian Noack wrote:
> Nit; from https://adblockplus.org/coding-style#python:
> 
>   Use the + operator when concatenating exactly two strings, ...

Done.

https://codereview.adblockplus.org/29501558/diff/29550602/tests/tools.py
File tests/tools.py (right):

https://codereview.adblockplus.org/29501558/diff/29550602/tests/tools.py#newc...
tests/tools.py:125: if not build_type == 'release':
On 2017/09/20 21:36:12, Sebastian Noack wrote:
> Nit: `a != b` instead of `not a == b`
> 
> Is flake8 checking this file?

It is - along with all plugins. Since there is no entry in tox.ini to ignore
anything in this file, i'm curious too, why this was not marked.

This line will be deleted anyway, as soon as i adhere to your release build
strategy.

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md File tests/README.md (right): https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md#newcode58 tests/README.md:58: `tox` On 2017/09/21 09:29:02, Vasily Kuznetsov wrote: > On ...

Sept. 21, 2017, 7:09 p.m. (2017-09-21 19:09:08 UTC) #35

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md
File tests/README.md (right):

https://codereview.adblockplus.org/29501558/diff/29549678/tests/README.md#new...
tests/README.md:58: `tox`
On 2017/09/21 09:29:02, Vasily Kuznetsov wrote:
> On 2017/09/20 21:36:09, Sebastian Noack wrote:
> > On 2017/09/20 14:53:04, Vasily Kuznetsov wrote:
> > > On 2017/09/20 00:58:01, Sebastian Noack wrote:
> > > > On 2017/09/19 17:52:26, Vasily Kuznetsov wrote:
> > > > > You can use a 4 space indent here instead, and I would also suggest
> adding
> > a
> > > > > shell prompt before "tox" so it's clear that we run it from shell
> prompt.
> > > > > Something like this:
> > > > > 
> > > > >     $ tox
> > > > 
> > > > Actually, since the whole paragraph consists of code, you should rather
> use
> > > > triple backticks and no indentation at all:
> > > > 
> > > > ```
> > > > $ tox
> > > > ```
> > > 
> > > Seems like it's more cluttered and harder to read source code that
produces
> > the
> > > same output. Why do you think this is better than just a simple code
block?
> > 
> > The output is usually not the same (it might depend on the markdown renderer
> > though). Single backticks usually (e.g. on GitHub) don't render as code
block,
> > but render a code snippet, and are meant for code inside a text paragraph.
> 
> I agree about the single backticks, but my proposal was a code block
> (https://daringfireball.net/projects/markdown/syntax#precode).

Oh, I didn't know of that syntax, I was assuming you were suggesting to keep the
single backticks but just change the indentation. But indeed indented code is
rendered the same as code wrapped in triple backticks, but reads nicer in plain
text. The only downside seems to be that you cannot annotate the programming
language for code hightlighting, but that doesn't matter here.

Tristan, so if you want to go with Vasily's suggestion, fine with me.

https://codereview.adblockplus.org/29501558/diff/29551604/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29501558/diff/29551604/packagerChrome.py#n...
packagerChrome.py:54: raise TypeError(filename + ' is no valid PNG.')
The definition of TypeError doesn't seem to apply here:
https://docs.python.org/3/library/exceptions.html#TypeError

This would rather be a ValueError. But for consistency, I would just go with
Exception, which is what buildtools is raising in other scenarios like this.

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
File tests/test_packagerEdge.py (right):

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
tests/test_packagerEdge.py:146: filename =
'{{}}_edge_{}_{}.{{}}'.format(release, buildnum)
The buildnum is redundant here. The filenames are still distinct if you remove
that part.

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
tests/test_packagerWebExt.py:135: assert 'qunit/index.html' not in filenames
Neither should 'devenvPoller__.js' and devenvVersion__' be in the build. In
order to avoid duplication, you can do this like that:

  assert devenv == ('qunit/index.html' in filenames)
  assert devenv == ('devenvPoller__.js' in filenames)
  assert devenv == ('devenvVersion__' in filenames)

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
tests/test_packagerWebExt.py:219: 'manifest_{}_{}_{}.json'.format(platform,
dev_build_release, buildnum),
The buildnum is redundant here. The filenames are still distinct if you remove
that part.

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
tests/test_packagerWebExt.py:254: assert_base_files(package)
The features tested here are relevant for "edge" builds as well, yet we only
test them "chrome" and "gecko-webext"?

FWIW, I don't see why the tests have been separated into test_packagerWebExt.py
and test_packagerEdge.py in the first place.

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packagerEdge.py File tests/test_packagerEdge.py (right): https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packagerEdge.py#newcode146 tests/test_packagerEdge.py:146: filename = '{{}}_edge_{}_{}.{{}}'.format(release, buildnum) On 2017/09/21 19:09:04, Sebastian Noack ...

Sept. 22, 2017, 1:56 a.m. (2017-09-22 01:56:08 UTC) #36

tlucas

Patch Set 13 * Removing redundant buildnum in filenames * Raise Exception instead of TypeError ...

Sept. 22, 2017, 9:02 a.m. (2017-09-22 09:02:23 UTC) #37

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packagerWebExt.py File tests/test_packagerWebExt.py (right): https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packagerWebExt.py#newcode254 tests/test_packagerWebExt.py:254: assert_base_files(package) On 2017/09/22 09:02:18, tlucas wrote: > On 2017/09/21 ...

Sept. 22, 2017, 6:31 p.m. (2017-09-22 18:31:06 UTC) #38

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
tests/test_packagerWebExt.py:254: assert_base_files(package)
On 2017/09/22 09:02:18, tlucas wrote:
> On 2017/09/21 19:09:06, Sebastian Noack wrote:
> > The features tested here are relevant for "edge" builds as well, yet we only
> > test them "chrome" and "gecko-webext"?
> > 
> > FWIW, I don't see why the tests have been separated into
> test_packagerWebExt.py
> > and test_packagerEdge.py in the first place.
> 
> This test happens in edge as well, see the 7 single assertions after
> "assert_all_locales_present()" in test_build_edge. I would agree on moving
them
> to a function.

The logic there seems inconsistent. It seems that either the test build for
"edge" includes less files (and is using less feature), or that you just don't
check for the same files as you do here. Neither makes any sense to me.

Also I wasn't specifically talking about assert_base_files(), but about all the
assert_*() functions here. The same features tested here, are also supported by
the "edge" target.

> Regarding the two separate files, imho it makes sense to create one test file
> for one module, which would be packagerEdge and packagerChrome.

These aren't unit tests for the packager modules, but integration tests for the
command line interface. (Or alternatively you could argue that the module tested
here is build.py, and not packager*.py). That most of the logic for the "chrome"
and "gecko-webext" targets are implemented in packagerChrome.py, and most of the
logic for the "edge" target is implemented in packagerEdge.py, are
implementation details that aren't relevant on the level we are testing here.

It seems it would simplify the tests a bit, and would make it easier to avoid
unintended inconsistencies if we merge those test modules.

https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py
File tests/tools.py (right):

https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py#newc...
tests/tools.py:187: diff = list(difflib.unified_diff(json_a, json_b, n=0))
On 2017/09/22 09:02:20, tlucas wrote:
> On 2017/09/22 01:56:05, Sebastian Noack wrote:
> > This logic is duplicated. Why not just move it to assert_manifest_content()?
> 
> See
>
https://codereview.adblockplus.org/29501558/diff2/29541651:29543665/tests/too...
> in LEFT line 177 - if you don't mind, i would stick to this.

I don't see how that discussion is related to the suggestion here. I am merely
suggesting to make compare_{xml|json} return a and b, so that we can move the
duplicated code to assert_manifest_content(). This doesn't seem to contradict
anything discussed earlier.

tlucas

On 2017/09/22 18:31:06, Sebastian Noack wrote: > https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py > File tests/tools.py (right): > > https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py#newcode187 ...

Sept. 23, 2017, 8:42 a.m. (2017-09-23 08:42:39 UTC) #39

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py File tests/tools.py (right): https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py#newcode187 tests/tools.py:187: diff = list(difflib.unified_diff(json_a, json_b, n=0)) On 2017/09/23 08:42:39, tlucas ...

Sept. 23, 2017, 9:11 p.m. (2017-09-23 21:11:42 UTC) #40

https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py
File tests/tools.py (right):

https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py#newc...
tests/tools.py:187: diff = list(difflib.unified_diff(json_a, json_b, n=0))
On 2017/09/23 08:42:39, tlucas wrote:
> On 2017/09/22 18:31:06, Sebastian Noack wrote:
> > https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py
> > File tests/tools.py (right):
> > 
> >
>
https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py#newc...
> > tests/tools.py:187: diff = list(difflib.unified_diff(json_a, json_b, n=0))
> > On 2017/09/22 09:02:20, tlucas wrote:
> > > On 2017/09/22 01:56:05, Sebastian Noack wrote:
> > > > This logic is duplicated. Why not just move it to
> assert_manifest_content()?
> > > 
> > > See
> > >
> >
>
https://codereview.adblockplus.org/29501558/diff2/29541651:29543665/tests/too...
> > > in LEFT line 177 - if you don't mind, i would stick to this.
> > 
> > I don't see how that discussion is related to the suggestion here. I am
merely
> > suggesting to make compare_{xml|json} return a and b, so that we can move
the
> > duplicated code to assert_manifest_content(). This doesn't seem to
contradict
> > anything discussed earlier.
> 
> Sorry, this is an error on my side - i meant line 174:
> 
> On 2017/09/11 12:50:08, Vasily Kuznetsov wrote:
> > This function and the one below seems to be only used in assertions. It's
> > probably better to just move the `assert`s into the functions -- this way
> pytest
> > will produce more helpful error messages with the diffs between the things
> we're
> > comparing.

If you just make make compare_{xml|json} return a and b (as I suggested above),
and then move following to compare_json(), I don't see how this would give any
less useful error message:

  diff = list(difflib.unified_diff(a, b, n=0))
  assert len(diff) == 0, '\n'.join(diff)

In the original code, you did the comparison before using the assert statement,
so the error message would only show False, rather than the expected and actual
result that are compared. But that isn't the case if you do it like I suggest.

tlucas

On 2017/09/22 18:31:06, Sebastian Noack wrote: > https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packagerWebExt.py > File tests/test_packagerWebExt.py (right): > > https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packagerWebExt.py#newcode254 ...

Sept. 25, 2017, 8:34 a.m. (2017-09-25 08:34:21 UTC) #41

On 2017/09/22 18:31:06, Sebastian Noack wrote:
>
https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
> File tests/test_packagerWebExt.py (right):
> 
>
https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
> tests/test_packagerWebExt.py:254: assert_base_files(package)
> On 2017/09/22 09:02:18, tlucas wrote:
> > On 2017/09/21 19:09:06, Sebastian Noack wrote:
> > > The features tested here are relevant for "edge" builds as well, yet we
only
> > > test them "chrome" and "gecko-webext"?
> > > 
> > > FWIW, I don't see why the tests have been separated into
> > test_packagerWebExt.py
> > > and test_packagerEdge.py in the first place.
> > 
> > This test happens in edge as well, see the 7 single assertions after
> > "assert_all_locales_present()" in test_build_edge. I would agree on moving
> them
> > to a function.
> 
> The logic there seems inconsistent. It seems that either the test build for
> "edge" includes less files (and is using less feature), or that you just don't
> check for the same files as you do here. Neither makes any sense to me.
> 
> Also I wasn't specifically talking about assert_base_files(), but about all
the
> assert_*() functions here. The same features tested here, are also supported
by
> the "edge" target.

Will do (for possible tests).

> > Regarding the two separate files, imho it makes sense to create one test
file
> > for one module, which would be packagerEdge and packagerChrome.
> 
> These aren't unit tests for the packager modules, but integration tests for
the
> command line interface. (Or alternatively you could argue that the module
tested
> here is build.py, and not packager*.py). That most of the logic for the
"chrome"
> and "gecko-webext" targets are implemented in packagerChrome.py, and most of
the
> logic for the "edge" target is implemented in packagerEdge.py, are
> implementation details that aren't relevant on the level we are testing here.
> 
> It seems it would simplify the tests a bit, and would make it easier to avoid
> unintended inconsistencies if we merge those test modules.

Okay, i will merge the two files.
Since modularization of shared functions is then not necessary anymore, I could
also get rid of /tests/__init__.py, tests/tools.py and tests/conftest.py -
although this could kill overview and readability.
What do you think about this?

On 2017/09/23 21:11:42, Sebastian Noack wrote:
> https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py
> File tests/tools.py (right):
> 
>
https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py#newc...
> tests/tools.py:187: diff = list(difflib.unified_diff(json_a, json_b, n=0))
> On 2017/09/23 08:42:39, tlucas wrote:
> > On 2017/09/22 18:31:06, Sebastian Noack wrote:
> > > https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py
> > > File tests/tools.py (right):
> > > 
> > >
> >
>
https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py#newc...
> > > tests/tools.py:187: diff = list(difflib.unified_diff(json_a, json_b, n=0))
> > > On 2017/09/22 09:02:20, tlucas wrote:
> > > > On 2017/09/22 01:56:05, Sebastian Noack wrote:
> > > > > This logic is duplicated. Why not just move it to
> > assert_manifest_content()?
> > > > 
> > > > See
> > > >
> > >
> >
>
https://codereview.adblockplus.org/29501558/diff2/29541651:29543665/tests/too...
> > > > in LEFT line 177 - if you don't mind, i would stick to this.
> > > 
> > > I don't see how that discussion is related to the suggestion here. I am
> merely
> > > suggesting to make compare_{xml|json} return a and b, so that we can move
> the
> > > duplicated code to assert_manifest_content(). This doesn't seem to
> contradict
> > > anything discussed earlier.
> > 
> > Sorry, this is an error on my side - i meant line 174:
> > 
> > On 2017/09/11 12:50:08, Vasily Kuznetsov wrote:
> > > This function and the one below seems to be only used in assertions. It's
> > > probably better to just move the `assert`s into the functions -- this way
> > pytest
> > > will produce more helpful error messages with the diffs between the things
> > we're
> > > comparing.
> 
> If you just make make compare_{xml|json} return a and b (as I suggested
above),
> and then move following to compare_json(), I don't see how this would give any
> less useful error message:
> 
>   diff = list(difflib.unified_diff(a, b, n=0))
>   assert len(diff) == 0, '\n'.join(diff)
> 
> In the original code, you did the comparison before using the assert
statement,
> so the error message would only show False, rather than the expected and
actual
> result that are compared. But that isn't the case if you do it like I suggest.

Ok, now i see what you mean. I agree and i would suggest to rename
compare_{xml|json} to comparable_{xml|json} and make them accept one argument
and return one nonambiguous representation.
assert_manifest_content() will then do the assertion itself. What do you think?

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packagerWebExt.py File tests/test_packagerWebExt.py (right): https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packagerWebExt.py#newcode254 tests/test_packagerWebExt.py:254: assert_base_files(package) On 2017/09/25 08:34:21, tlucas wrote: > On 2017/09/22 ...

Sept. 25, 2017, 8:21 p.m. (2017-09-25 20:21:23 UTC) #42

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29551604/tests/test_packager...
tests/test_packagerWebExt.py:254: assert_base_files(package)
On 2017/09/25 08:34:21, tlucas wrote:
> On 2017/09/22 18:31:06, Sebastian Noack wrote:
> > These aren't unit tests for the packager modules, but integration tests for
> the
> > command line interface. (Or alternatively you could argue that the module
> tested
> > here is build.py, and not packager*.py). That most of the logic for the
> "chrome"
> > and "gecko-webext" targets are implemented in packagerChrome.py, and most of
> the
> > logic for the "edge" target is implemented in packagerEdge.py, are
> > implementation details that aren't relevant on the level we are testing
here.
> > 
> > It seems it would simplify the tests a bit, and would make it easier to
avoid
> > unintended inconsistencies if we merge those test modules.
> 
> Okay, i will merge the two files.
> Since modularization of shared functions is then not necessary anymore, I
could
> also get rid of /tests/__init__.py, tests/tools.py and tests/conftest.py -
> although this could kill overview and readability.
> What do you think about this?

Yes, I agree that there is no reason to split out common functionality in other
modules if we unify the tests. I'd expect to unify the code path for testing
Chrome/Firefox/Edge builds for the most part (e.g. the entry point should be a
single parametized function for all targets), so that the amount of code will
decrease as well, so that hopefully the readability will not suffer.

https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py
File tests/tools.py (right):

https://codereview.adblockplus.org/29501558/diff/29551604/tests/tools.py#newc...
tests/tools.py:187: diff = list(difflib.unified_diff(json_a, json_b, n=0))
On 2017/09/25 08:34:21, tlucas wrote:
> Ok, now i see what you mean. I agree and i would suggest to rename
> compare_{xml|json} to comparable_{xml|json} and make them accept one argument
> and return one nonambiguous representation.
> assert_manifest_content() will then do the assertion itself. What do you
think?

Yes, that is essentially what I suggested.

tlucas

Patch Set 14 * Combined packager tests for Edge / Chrome / Firefox into test_packagerWebExt.py ...

Sept. 25, 2017, 10:38 p.m. (2017-09-25 22:38:11 UTC) #43

kzar

Nice one for adding all these tests and even documenting them. I'm not the best ...

Oct. 16, 2017, 11:17 a.m. (2017-10-16 11:17:32 UTC) #44

tlucas

Patch Set 18 * Addressing Dave's comments from PS 16 * Mentioned webpack in tests/README.md ...

Oct. 17, 2017, 12:45 p.m. (2017-10-17 12:45:52 UTC) #45

tlucas

Just for your interest: The tests currently take ~ 20 seconds, versus ~ 5 seconds ...

Oct. 17, 2017, 12:47 p.m. (2017-10-17 12:47:48 UTC) #46

kzar

LGTM https://codereview.adblockplus.org/29501558/diff/29581658/ensure_dependencies.py File ensure_dependencies.py (left): https://codereview.adblockplus.org/29501558/diff/29581658/ensure_dependencies.py#oldcode428 ensure_dependencies.py:428: parser.add_argument('-q', '--quiet', action='store_true', help='Suppress informational output') On 2017/10/17 ...

Oct. 17, 2017, 1:03 p.m. (2017-10-17 13:03:57 UTC) #47

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29581658/ensure_dependencies.py File ensure_dependencies.py (left): https://codereview.adblockplus.org/29501558/diff/29581658/ensure_dependencies.py#oldcode428 ensure_dependencies.py:428: parser.add_argument('-q', '--quiet', action='store_true', help='Suppress informational output') On 2017/10/17 13:03:54, ...

Oct. 17, 2017, 10:19 p.m. (2017-10-17 22:19:13 UTC) #48

tlucas

Patch Set 20 * Including tests for #4720 * Adjusted tests for Edge to be ...

Oct. 18, 2017, 11:23 a.m. (2017-10-18 11:23:15 UTC) #49

Patch Set 20

 * Including tests for #4720
 * Adjusted tests for Edge to be more consistent with Chrome / Firefox
 * Reverting ensure_dependencies.py changes, calling 'npm install' explicitly
 * Addressing nits

@ Sebastion, please find the discussion below.

https://codereview.adblockplus.org/29501558/diff/29581658/ensure_dependencies.py
File ensure_dependencies.py (left):

https://codereview.adblockplus.org/29501558/diff/29581658/ensure_dependencies...
ensure_dependencies.py:428: parser.add_argument('-q', '--quiet',
action='store_true', help='Suppress informational output')
On 2017/10/17 22:19:06, Sebastian Noack wrote:
> On 2017/10/17 13:03:54, kzar wrote:
> > On 2017/10/17 12:45:50, tlucas wrote:
> > > The changes in this file are meant to enable tox to install webpack, in
> order
> > to
> > > verify the packaging.
> > > I'm not too sure about this though - please let me know what you think!
> > 
> > Yea I know what you mean, alternative I suppose tox could just run npm
> directly
> > but I'm not sure which is better. This looks OK to me anyway, I guess
> sometimes
> > stuff like this is necessary for tests.
> 
> Any particular reason, to not just call "npm install" directly in tox.ini?

What i had in mind was to have the ensure_npm_dependencies branch in
ensure_dependencies covered - but i guess this would be overkill right now and
should be done later, when i enhance the overall testcoverage.

Calling 'npm install' explicitly now.

https://codereview.adblockplus.org/29501558/diff/29581658/tests/README.md
File tests/README.md (right):

https://codereview.adblockplus.org/29501558/diff/29581658/tests/README.md#new...
tests/README.md:29: _(Covered platforms are referred to as C=Chrome, E=Edge,
F=Firefox)_
On 2017/10/17 13:03:54, kzar wrote:
> Nit: Missing full stop.

Done.

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
tests/test_packagerWebExt.py:19: 
On 2017/10/17 22:19:06, Sebastian Noack wrote:
> Nit: One blank line is enough here.

Done.

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
tests/test_packagerWebExt.py:143: if buildnum:
On 2017/10/17 22:19:09, Sebastian Noack wrote:
> It doesn't seem unneccessary to paremtize the build version, as it is always
> 1337 for development builds, and absent otherwise.

I assume you meant "It doesn't seem necessary", from what i understand from the
rest of this comment.

If i'm correct, please see the comment below, regarding the
prefix-parametrization.

If i'm wrong, could you please clarify?

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
tests/test_packagerWebExt.py:393: ('chrome', 'build', '1337'),
On 2017/10/17 22:19:07, Sebastian Noack wrote:
> The naming of the second parameter is somewhat misleading:
> 
> * "build" is development build
> * "release" is release build

Renamed it to "command", adhering to the help text in build.py.

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
tests/test_packagerWebExt.py:431: prefix = ''
On 2017/10/17 22:19:09, Sebastian Noack wrote:
> I wonder whether we should include stuff like (but not limited) to this, in
the
> parametization above, getting rid of unnecessary if-statements.

Assuming i was correct above:

You argued that we don't need parametrization for buildnum (since it is directly
dependent on whether we perform a development build or not)

Here you suggest that we introduce parametrization for 'prefix', which is
directly dependent on whether we we perform an edge-build or not.

I guess there are two options to choose from (but i think we should keep it
consistent):

a) Get rid of parametrization for everything that is directly dependent on an
already given parameter (resulting in more ifs).

b) Parametrize basically everything that would need further processing in the
parametrized functions.

I don't have a too strong opinion about this, but it might seem more natural
that parametrization solely is responsible for cycling through the combinations
of the actual desired tests (which in this case would be the combinations of
platform and command), hence option a).

For the sake of readability i could add another function (e.g.
process_parameters()) which could then, outside of run_webext_build(), prepare
those parameters that depend on the parametrization.

What do you think about this?

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
tests/test_packagerWebExt.py:438: if platform in {'chrome', 'gecko'}:
On 2017/10/17 22:19:08, Sebastian Noack wrote:
> Why is this specific to Chrome/Gecko?

It is not - i guess this is merely a relict from the time when i did not yet
have the full scope of metadata inheritance in mind.

Checking for all platforms now.

https://codereview.adblockplus.org/29501558/diff/29581658/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29501558/diff/29581658/tox.ini#newcode24
tox.ini:24: recreate = true
On 2017/10/17 22:19:10, Sebastian Noack wrote:
> Any reason we force the venv to be recreated on every run? This will slow down
> tox significantly.

This was an unintended leftover - removed it.

tlucas

Of course i meant "Sebastian", please excuse my typo.

Oct. 18, 2017, 11:23 a.m. (2017-10-18 11:23:56 UTC) #50

Sebastian Noack

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packagerWebExt.py File tests/test_packagerWebExt.py (right): https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packagerWebExt.py#newcode143 tests/test_packagerWebExt.py:143: if buildnum: On 2017/10/18 11:23:06, tlucas wrote: > On ...

Oct. 19, 2017, 5:37 a.m. (2017-10-19 05:37:57 UTC) #51

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
tests/test_packagerWebExt.py:143: if buildnum:
On 2017/10/18 11:23:06, tlucas wrote:
> On 2017/10/17 22:19:09, Sebastian Noack wrote:
> > It doesn't seem unneccessary to paremtize the build version, as it is always
> > 1337 for development builds, and absent otherwise.
> 
> I assume you meant "It doesn't seem necessary", from what i understand from
the
> rest of this comment.
> 
> If i'm correct, please see the comment below, regarding the
> prefix-parametrization.
> 
> 
> If i'm wrong, could you please clarify?

Sorry for the confusion, I commented below. Either way, the build number can be
hard-coded as part of the command line whichever way we go.

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
tests/test_packagerWebExt.py:393: ('chrome', 'build', '1337'),
On 2017/10/18 11:23:11, tlucas wrote:
> On 2017/10/17 22:19:07, Sebastian Noack wrote:
> > The naming of the second parameter is somewhat misleading:
> > 
> > * "build" is development build
> > * "release" is release build
> 
> Renamed it to "command", adhering to the help text in build.py.

I wasn't talking about the name of the argument variable, though that one was
bogus too, but about its values. "build" is misleading  as both "build" and
"release" are builds, but the former is the development build, while the latter
is the release build.

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
tests/test_packagerWebExt.py:431: prefix = ''
On 2017/10/18 11:23:08, tlucas wrote:
> On 2017/10/17 22:19:09, Sebastian Noack wrote:
> > I wonder whether we should include stuff like (but not limited) to this, in
> the
> > parametization above, getting rid of unnecessary if-statements.
> 
> Assuming i was correct above:
> 
> You argued that we don't need parametrization for buildnum (since it is
directly
> dependent on whether we perform a development build or not)
> 
> Here you suggest that we introduce parametrization for 'prefix', which is
> directly dependent on whether we we perform an edge-build or not.
> 
> I guess there are two options to choose from (but i think we should keep it
> consistent):
> 
> a) Get rid of parametrization for everything that is directly dependent on an
> already given parameter (resulting in more ifs).
> 
> b) Parametrize basically everything that would need further processing in the
> parametrized functions.
> 
> I don't have a too strong opinion about this, but it might seem more natural
> that parametrization solely is responsible for cycling through the
combinations
> of the actual desired tests (which in this case would be the combinations of
> platform and command), hence option a).
> 
> For the sake of readability i could add another function (e.g.
> process_parameters()) which could then, outside of run_webext_build(), prepare
> those parameters that depend on the parametrization.
> 
> What do you think about this?

Sorry, I actually thought that I deleted this comment before submitting. I'm not
too happy with all those ifs in the logic here. It makes it difficult to verify
that the right things get tested, and in general, abstraction is better than
exceptions (or spaghetti code). But for the matter of progress, I wouldn't
necessarily insist on a refactoring at the current point.

https://codereview.adblockplus.org/29501558/diff/29582593/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29582593/tests/test_packager...
tests/test_packagerWebExt.py:331: if platform in {'chrome', 'gecko'}:
While, the files checked in the else-clause are extra foo specific to Edge, any
reason why there are any files that are not expected to be in the Edge builds?

tlucas

Patch Set 21 * Refactored command values (as discussed below) * Asserting presence of all ...

Oct. 20, 2017, 12:05 p.m. (2017-10-20 12:05:22 UTC) #52

Patch Set 21

 * Refactored command values (as discussed below)
 * Asserting presence of all appropriate files on all platforms

On 2017/10/19 05:37:57, Sebastian Noack wrote:
>
https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
> File tests/test_packagerWebExt.py (right):
> 
>
https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
> tests/test_packagerWebExt.py:143: if buildnum:
> On 2017/10/18 11:23:06, tlucas wrote:
> > On 2017/10/17 22:19:09, Sebastian Noack wrote:
> > > It doesn't seem unneccessary to paremtize the build version, as it is
always
> > > 1337 for development builds, and absent otherwise.

Please see below.

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
> tests/test_packagerWebExt.py:393: ('chrome', 'build', '1337'),
> On 2017/10/18 11:23:11, tlucas wrote:
> > On 2017/10/17 22:19:07, Sebastian Noack wrote:
> > > The naming of the second parameter is somewhat misleading:
> > > 
> > > * "build" is development build
> > > * "release" is release build
> > 
> > Renamed it to "command", adhering to the help text in build.py.
> 
> I wasn't talking about the name of the argument variable, though that one was
> bogus too, but about its values. "build" is misleading  as both "build" and
> "release" are builds, but the former is the development build, while the
latter
> is the release build.

I renamed the values and introduced a mapping for them (which also offered a
rather elegant way to hardcode the buildnum)

https://codereview.adblockplus.org/29501558/diff/29581658/tests/test_packager...
> tests/test_packagerWebExt.py:431: prefix = ''
> On 2017/10/18 11:23:08, tlucas wrote:
> > On 2017/10/17 22:19:09, Sebastian Noack wrote:
> > > I wonder whether we should include stuff like (but not limited) to this,
in
> > > ...
> > 
> > Assuming i was correct above:
> > ...
> 
> Sorry, I actually thought that I deleted this comment before submitting. I'm
not
> too happy with all those ifs in the logic here. It makes it difficult to
verify
> that the right things get tested, and in general, abstraction is better than
> exceptions (or spaghetti code). But for the matter of progress, I wouldn't
> necessarily insist on a refactoring at the current point.


Acknowledged, i'll stick to how it is.

https://codereview.adblockplus.org/29501558/diff/29582593/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29582593/tests/test_packager...
tests/test_packagerWebExt.py:331: if platform in {'chrome', 'gecko'}:
On 2017/10/19 05:37:54, Sebastian Noack wrote:
> While, the files checked in the else-clause are extra foo specific to Edge,
any
> reason why there are any files that are not expected to be in the Edge builds?

No, changed it.

Vasily Kuznetsov

LGTM with a couple of nits: - PEP-8 recommends the following order of imports: stdlib, ...

Oct. 21, 2017, 5:41 p.m. (2017-10-21 17:41:15 UTC) #54

tlucas

Patch Set 22 Hey there! This patch does not contain changes in functionality. * Clarified ...

Oct. 22, 2017, 11:19 a.m. (2017-10-22 11:19:19 UTC) #55

Patch Set 22

Hey there!

This patch does not contain changes in functionality.

 * Clarified limitations of comparable_xml().
 * reordered some imports.
 * added node_modules to .{hg|git}ignore

On 2017/10/21 17:41:15, Vasily Kuznetsov wrote:
> LGTM with a couple of nits:
> 
> - PEP-8 recommends the following order of imports: stdlib, 3rd party
libraries,
> local imports (see below).
> - Be careful with sorting XML files although it's probably ok here (see
below).
> - The diffs of the images (tests/abp-16.png, etc.) are not in the patch that
was
> uploaded to Rietveld [1], so this change can't be pushed from the review. This
> is probably mostly my fault because I didn't make sure that the change from
> #5644 [2] landed. I've just reminded Matze about it. Anyway, you can patch
your
> own upload.py from the review for now [3].
>

I had this already integrated, but a reset purged your changes. The images
should have been uploaded correctly now.

> 
> Cheers,
> Vasily
> 
> P.S. You're welcome for another opportunity to cement your domination in the
> "biggest review" contest ;)
> 
> [1]: https://codereview.adblockplus.org/download/issue29501558_29584567.diff
> [2]: https://issues.adblockplus.org/ticket/5644
> [3]: https://codereview.adblockplus.org/29539660/

https://codereview.adblockplus.org/29501558/diff/29584567/tests/test_packager...
File tests/test_packagerEdge.py (right):

https://codereview.adblockplus.org/29501558/diff/29584567/tests/test_packager...
tests/test_packagerEdge.py:7: import xml.etree.ElementTree as ET
On 2017/10/21 17:41:09, Vasily Kuznetsov wrote:
> This should be imported before pytest: xml.etree is stdlib, but pytest is 3rd
> party.

Done.

https://codereview.adblockplus.org/29501558/diff/29584567/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29501558/diff/29584567/tests/test_packager...
tests/test_packagerWebExt.py:17: from xml.etree import ElementTree
On 2017/10/21 17:41:10, Vasily Kuznetsov wrote:
> The imports should be sorted+grouped in the following order: stdlib, 3rd party
> libraries, local imports [1].
> 
> [1] PEP-8 section on imports:
https://www.python.org/dev/peps/pep-0008/#imports.

Done.

https://codereview.adblockplus.org/29501558/diff/29584567/tests/test_packager...
tests/test_packagerWebExt.py:243: def comparable_xml(xml):
On 2017/10/21 17:41:11, Vasily Kuznetsov wrote:
> I suppose it doesn't matter for the purposes of this test suite, but you might
> want to note that this function is not quite bullet proof in terms of false
> positives. For example, take the following two XML files:
> 
> [a.xml]
> <a>
>   <b><c/></b>
>   <b><d/></b>
> </a>
> 
> [b.xml]
> <a>
>   <b/>
>   <b><c/><d/></b>
> </a>
> 
> I don't think anybody would say that they are equivalent: clearly their
> structure is rather different and it's not only about the order of the tags.
> However, both a.xml and b.xml convert to:
> 
> a|None|None
> a|None|None__b|None|None
> a|None|None__b|None|None
> a|None|None__b|None|None__c|None|None
> a|None|None__b|None|None__d|None|None

I adjusted the docstring to clarify this limitation (the refactoring will be
done in a separate review)

https://codereview.adblockplus.org/29501558/diff/29584567/tox.ini
File tox.ini (right):

https://codereview.adblockplus.org/29501558/diff/29584567/tox.ini#newcode38
tox.ini:38: npm install --no-package-lock --only=production
On 2017/10/21 17:41:12, Vasily Kuznetsov wrote:
> This creates `node_modules` folder that should probably be ignored.

I thought this was handled by #5559 - but it would only be ignored if it was
added by ensure_dependencies.py (which was true for my repository recently, so
it was ignored for me).

Added it to .{hg|git}ignore.

| |      / __|   |_   _| |  \/  | 
  | |__   | (_ |     | |   | |\/| | 
  |____|   \___|    _|_|_  |_|__|_| 
_|"""""| _|"""""| _|"""""| _|"""""| 
"`-0-0-' "`-0-0-' "`-0-0-' "`-0-0-'

Issue 29501558: Issue 5383 - Add tests for the Chrome and Firefox packagers (Closed)

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : Addressing pep8 error from last review #

Patch Set 4 : Adding pytest-cov, adhering to comments #

Patch Set 5 : Refactoring tests, focussing on webext (chrome, gecko, edge) #

Patch Set 6 : Reuploading binary files after upload.py patch #

Patch Set 7 : Completely purge PIL / Pillow, added edge-extension fixture / assert #

Patch Set 8 : Readme, difflib, buildnum #

Patch Set 9 : Edge's manifest.json #

Patch Set 10 : Asserting PNGs #

Patch Set 11 : Adressing comments #

Patch Set 12 : Simplifying parameters #

Patch Set 13 : Adressing comments #

Patch Set 14 : Merge test were possible #

Patch Set 15 : Rebase against master, renaming gecko-webext to gecko #

Patch Set 16 : Adjusting tests to recent changes #

Patch Set 17 : Rebasing against current master (502:7e896c368056) #

Patch Set 18 : Addressing Dave's comments #

Patch Set 19 : NO CHANGE Rebasing against #4720 @ PS 4 #

Patch Set 20 : Addressing comments, handling Edge devenv #

Patch Set 21 : Refactoring commands #

Patch Set 22 : Addressing Vasily's comments #

Messages