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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 3 weeks ago by tlucas
Modified:
16 hours, 53 minutes ago
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: 15

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 #

Total comments: 18

Patch Set 8 : Readme, difflib, buildnum #

Total comments: 5

Patch Set 9 : Edge's manifest.json #

Total comments: 1

Patch Set 10 : Asserting PNGs #

Total comments: 19

Patch Set 11 : Adressing comments #

Total comments: 6

Patch Set 12 : Simplifying parameters #

Total comments: 18

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 #

Total comments: 20

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

Patch Set 18 : Addressing Dave's comments #

Total comments: 21

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

Patch Set 20 : Addressing comments, handling Edge devenv #

Total comments: 2

Patch Set 21 : Refactoring commands #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1303 lines, -245 lines) Patch
M .gitignore View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M .hgignore View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -0 lines 0 comments Download
M README.md View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +3 lines, -0 lines 0 comments Download
M package.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -1 line 0 comments Download
M packagerChrome.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +7 lines, -6 lines 0 comments Download
A tests/README.md View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +73 lines, -0 lines 0 comments Download
A tests/abp-16.png View 1 2 3 4 6 7 8 9 10 11 12 13 14 Binary file 0 comments Download
A tests/abp-19.png View 1 2 3 4 6 7 8 9 10 11 12 13 14 Binary file 0 comments Download
A tests/abp-53.png View 1 2 3 4 6 7 8 9 10 11 12 13 14 Binary file 0 comments Download
A tests/chrome_rsa.pem View 1 2 3 1 chunk +27 lines, -0 lines 0 comments Download
A + tests/expecteddata/AppxManifest_edge_development_build.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +16 lines, -16 lines 0 comments Download
A + tests/expecteddata/AppxManifest_edge_devenv.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +16 lines, -16 lines 0 comments Download
A + tests/expecteddata/AppxManifest_edge_release_build.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +16 lines, -16 lines 0 comments Download
A tests/expecteddata/manifest_chrome_development_build.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +62 lines, -0 lines 0 comments Download
A tests/expecteddata/manifest_chrome_devenv.json View 1 2 3 4 8 9 10 11 12 1 chunk +63 lines, -0 lines 0 comments Download
A tests/expecteddata/manifest_chrome_release_build.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +62 lines, -0 lines 0 comments Download
A tests/expecteddata/manifest_edge_development_build.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +62 lines, -0 lines 0 comments Download
A tests/expecteddata/manifest_edge_devenv.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +63 lines, -0 lines 0 comments Download
A tests/expecteddata/manifest_edge_release_build.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +62 lines, -0 lines 0 comments Download
A tests/expecteddata/manifest_gecko_development_build.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +69 lines, -0 lines 0 comments Download
A tests/expecteddata/manifest_gecko_devenv.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +69 lines, -0 lines 0 comments Download
A tests/expecteddata/manifest_gecko_release_build.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +68 lines, -0 lines 0 comments Download
A tests/metadata.chrome View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +38 lines, -0 lines 0 comments Download
M tests/metadata.edge View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +18 lines, -10 lines 0 comments Download
A tests/metadata.gecko View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +13 lines, -0 lines 0 comments Download
M tests/test_packagerEdge.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +6 lines, -178 lines 0 comments Download
A tests/test_packagerWebExt.py View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +474 lines, -0 lines 0 comments Download
M tox.ini View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +11 lines, -2 lines 0 comments Download

Messages

Total messages: 52
tlucas
Patch Set 1 * Outsourced n>1-times used fixtures into conftest.py * Adjusted py.test config to ...
2 months, 3 weeks 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 ...
2 months, 3 weeks 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 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks 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 months, 2 weeks ago (2017-08-03 21:26:03 UTC) #6
tlucas
Path Set 2 * Added example manifests for edge and gecko to check against * ...
2 months, 2 weeks 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 ...
2 months, 1 week ago (2017-08-10 19:48:29 UTC) #8
tlucas
Path Set 3 * renaming files to base_files * adding missing commas to some lines ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week ago (2017-08-14 14:23:18 UTC) #11
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, ...
2 months, 1 week ago (2017-08-14 15:20:13 UTC) #12
tlucas
Patch Set 5 Hi everyone. This is a rather huge change, compared to Patch Set ...
1 month, 2 weeks ago (2017-09-06 09:04:53 UTC) #13
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, ...
1 month, 2 weeks ago (2017-09-06 18:00:01 UTC) #14
Vasily Kuznetsov
Hi Tristan! I didn't get the time to look through all the code thoroughly, so ...
1 month, 1 week ago (2017-09-07 20:27:51 UTC) #15
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 ...
1 month, 1 week ago (2017-09-07 20:39:00 UTC) #16
tlucas
Patch Set 6 * reuploaded images (upload.py can now properly handle binary files, credits to ...
1 month, 1 week ago (2017-09-08 10:31:28 UTC) #17
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: > ...
1 month, 1 week ago (2017-09-08 21:07:12 UTC) #18
tlucas
Patch Set 7: * purge PIL / Pillow dependency * Additional test for "move_files_to_extension" in ...
1 month, 1 week ago (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 ...
1 month, 1 week ago (2017-09-11 12:50:09 UTC) #20
tlucas
Hey Vasily, thanks for your comments! On 2017/09/11 12:50:09, Vasily Kuznetsov wrote: > Hi Tristan, ...
1 month, 1 week ago (2017-09-12 11:32:13 UTC) #21
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: > ...
1 month, 1 week ago (2017-09-12 21:55:27 UTC) #22
tlucas
Patch Set 8 Crucial: Please note that this new patch set is implemented on top ...
1 month, 1 week ago (2017-09-13 13:43:27 UTC) #23
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 ...
1 month, 1 week ago (2017-09-13 19:18:30 UTC) #24
tlucas
Patcht Set 9 Important: Please note that this new patch set is implemented on top ...
1 month ago (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 ...
1 month ago (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 ...
1 month ago (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 ...
1 month ago (2017-09-19 17:52:28 UTC) #28
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 ...
1 month ago (2017-09-20 00:58:05 UTC) #29
tlucas
Patch Set 11 * Removing unsafe 'assert', raising a TypeError instead * Adjusting content of ...
1 month ago (2017-09-20 08:52:54 UTC) #30
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 ...
1 month ago (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 ...
1 month ago (2017-09-20 21:36:14 UTC) #32
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 ...
4 weeks, 1 day ago (2017-09-21 09:29:03 UTC) #33
tlucas
Patch Set 12 * Called "./build.py ... build -r" instead of "./build.py ... release" * ...
4 weeks, 1 day ago (2017-09-21 11:35:00 UTC) #34
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 ...
4 weeks, 1 day ago (2017-09-21 19:09:08 UTC) #35
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 ...
4 weeks, 1 day ago (2017-09-22 01:56:08 UTC) #36
tlucas
Patch Set 13 * Removing redundant buildnum in filenames * Raise Exception instead of TypeError ...
4 weeks ago (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 ...
4 weeks ago (2017-09-22 18:31:06 UTC) #38
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 ...
3 weeks, 6 days ago (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 ...
3 weeks, 6 days ago (2017-09-23 21:11:42 UTC) #40
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 ...
3 weeks, 4 days ago (2017-09-25 08:34:21 UTC) #41
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 ...
3 weeks, 4 days ago (2017-09-25 20:21:23 UTC) #42
tlucas
Patch Set 14 * Combined packager tests for Edge / Chrome / Firefox into test_packagerWebExt.py ...
3 weeks, 4 days ago (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 ...
4 days, 17 hours ago (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 ...
3 days, 16 hours ago (2017-10-17 12:45:52 UTC) #45
tlucas
Just for your interest: The tests currently take ~ 20 seconds, versus ~ 5 seconds ...
3 days, 16 hours ago (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 ...
3 days, 15 hours ago (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, ...
3 days, 6 hours ago (2017-10-17 22:19:13 UTC) #48
tlucas
Patch Set 20 * Including tests for #4720 * Adjusted tests for Edge to be ...
2 days, 17 hours ago (2017-10-18 11:23:15 UTC) #49
tlucas
Of course i meant "Sebastian", please excuse my typo.
2 days, 17 hours ago (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 ...
1 day, 23 hours ago (2017-10-19 05:37:57 UTC) #51
tlucas
16 hours, 53 minutes ago (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.
Sign in to reply to this message.

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