Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

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

Created:
July 31, 2017, 12:07 p.m. by tlucas
Modified:
Oct. 22, 2017, 11:33 a.m.
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 #

Total comments: 8

Patch Set 22 : Addressing Vasily's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1314 lines, -243 lines) Patch
M .gitignore View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +3 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 18 19 20 21 1 chunk +3 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 5 15 16 17 18 19 20 21 Binary file 0 comments Download
A tests/abp-19.png View 1 2 3 4 5 15 16 17 18 19 20 21 Binary file 0 comments Download
A tests/abp-53.png View 1 2 3 4 5 15 16 17 18 19 20 21 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 14 15 16 17 18 19 20 21 2 chunks +4 lines, -176 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 21 1 chunk +485 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: 56
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Sebastian Noack
LGTM
Oct. 21, 2017, 7:08 a.m. (2017-10-21 07:08:12 UTC) #53
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
Vasily Kuznetsov
Oct. 22, 2017, 11:29 a.m. (2017-10-22 11:29:30 UTC) #56
| |      / __|   |_   _| |  \/  | 
  | |__   | (_ |     | |   | |\/| | 
  |____|   \___|    _|_|_  |_|__|_| 
_|"""""| _|"""""| _|"""""| _|"""""| 
"`-0-0-' "`-0-0-' "`-0-0-' "`-0-0-'

Powered by Google App Engine
This is Rietveld