Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(782)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 1 day ago by tlucas
Modified:
1 week, 1 day ago
CC:
kzar
Visibility:
Public.

Description

Issue 5383 - Add tests for the Chrome and Firefox packagers

Patch Set 1 #

Total comments: 96

Patch Set 2 : #

Total comments: 15

Patch Set 3 : Addressing pep8 error from last review #

Total comments: 20

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

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+1073 lines, -174 lines) Patch
M .gitignore View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
A + tests/__init__.py View 1 0 chunks +-1 lines, --1 lines 0 comments Download
A tests/chrome_rsa.pem View 1 2 3 1 chunk +27 lines, -0 lines 0 comments Download
A tests/conftest.py View 1 2 3 1 chunk +57 lines, -0 lines 0 comments Download
A + tests/expecteddata/manifest_edge_False.xml View 1 3 chunks +16 lines, -16 lines 0 comments Download
A + tests/expecteddata/manifest_edge_True.xml View 1 3 chunks +16 lines, -16 lines 0 comments Download
A tests/expecteddata/manifest_gecko_False_False.rdf View 1 1 chunk +57 lines, -0 lines 0 comments Download
A tests/expecteddata/manifest_gecko_False_True.rdf View 1 1 chunk +57 lines, -0 lines 0 comments Download
A tests/expecteddata/manifest_gecko_True_False.rdf View 1 1 chunk +57 lines, -0 lines 0 comments Download
A tests/expecteddata/manifest_gecko_True_True.rdf View 1 1 chunk +57 lines, -0 lines 0 comments Download
A tests/metadata.chrome View 1 chunk +47 lines, -0 lines 3 comments Download
A tests/metadata.gecko View 1 chunk +27 lines, -0 lines 1 comment Download
A tests/metadata.gecko-webext View 1 chunk +13 lines, -0 lines 0 comments Download
A tests/test_packagerChrome.py View 1 2 3 1 chunk +320 lines, -0 lines 0 comments Download
M tests/test_packagerEdge.py View 1 2 5 chunks +31 lines, -142 lines 0 comments Download
A tests/test_packagerGecko.py View 1 2 1 chunk +260 lines, -0 lines 0 comments Download
A tests/tools.py View 1 1 chunk +26 lines, -0 lines 0 comments Download
M tox.ini View 1 2 3 2 chunks +4 lines, -1 line 1 comment Download

Messages

Total messages: 12
tlucas
Patch Set 1 * Outsourced n>1-times used fixtures into conftest.py * Adjusted py.test config to ...
3 weeks, 1 day ago (2017-07-31 12:14:12 UTC) #1
kzar
Thanks for looking at this. Just so you know Sebastian is away at the moment ...
3 weeks, 1 day ago (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 ...
2 weeks, 6 days ago (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 ...
2 weeks, 6 days ago (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 ...
2 weeks, 5 days ago (2017-08-03 16:52:33 UTC) #5
tlucas
Hey Vasily, thanks for the review! I mostly agree with your comments, some comments require ...
2 weeks, 4 days ago (2017-08-03 21:26:03 UTC) #6
tlucas
Path Set 2 * Added example manifests for edge and gecko to check against * ...
2 weeks, 4 days ago (2017-08-04 14:52:01 UTC) #7
Vasily Kuznetsov
Hi Tristan, I'm sorry that I haven't replied to the review earlier: somehow it slipped ...
1 week, 4 days ago (2017-08-10 19:48:29 UTC) #8
tlucas
Path Set 3 * renaming files to base_files * adding missing commas to some lines ...
1 week, 4 days ago (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 ...
1 week, 4 days ago (2017-08-11 16:46:03 UTC) #10
tlucas
Patch Set 4 Added pytest-cov, outsourced / changed big constants (RSA-key, hash), enhanced readability according ...
1 week, 1 day ago (2017-08-14 14:23:18 UTC) #11
Sebastian Noack
1 week, 1 day ago (2017-08-14 15:20:13 UTC) #12
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
"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.

https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chro...
tests/metadata.chrome:23: ../lib/compat.js
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.

https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.chro...
tests/metadata.chrome:27: 
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.

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

https://codereview.adblockplus.org/29501558/diff/29515566/tests/metadata.geck...
tests/metadata.gecko:1: [general]
Support for legacy (non-web-ext) Gecko extensions will be obsolete very soon.
For the same reason we didn't add tests for Safari extensions. So I wonder
whether we should for legacy Gecko extensions?

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
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
      ...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5