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

Issue 29517687: Issue 5079, 5516 - Use webpack for browser tests, modules for content scripts (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 6 months ago by kzar
Modified:
2 years, 6 months ago
Reviewers:
Wladimir Palant, hub
Visibility:
Public.

Description

Issue 5079, 5516 - Use webpack for browser tests, modules for content scripts (This includes Hubert's earlier work https://codereview.adblockplus.org/29460576/ )

Patch Set 1 #

Total comments: 28

Patch Set 2 : Addressed Wladimir's and Hubert's initial feedback #

Total comments: 14

Patch Set 3 : runBrowserTests should always return a Promise #

Total comments: 2

Patch Set 4 : Addressed nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+191 lines, -728 lines) Patch
M README.md View 1 chunk +1 line, -1 line 0 comments Download
D chrome/content/.eslintrc.json View 1 chunk +0 lines, -5 lines 0 comments Download
D chrome/content/elemHideEmulation.js View 1 chunk +0 lines, -514 lines 0 comments Download
M chromium_process.js View 1 4 chunks +13 lines, -15 lines 0 comments Download
M lib/common.js View 1 chunk +41 lines, -2 lines 0 comments Download
A + lib/content/elemHideEmulation.js View 1 2 chunks +4 lines, -40 lines 0 comments Download
M package.json View 1 1 chunk +4 lines, -1 line 0 comments Download
M test/browser/_bootstrap.js View 1 1 chunk +58 lines, -103 lines 0 comments Download
M test/browser/elemHideEmulation.js View 1 4 chunks +4 lines, -27 lines 0 comments Download
M test_runner.js View 1 2 3 3 chunks +66 lines, -20 lines 0 comments Download

Messages

Total messages: 11
kzar
Patch Set 1 Got there in the end!
2 years, 6 months ago (2017-08-16 15:55:54 UTC) #1
hub
https://codereview.adblockplus.org/29517687/diff/29517688/lib/content/elemHideEmulation.js File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/29517687/diff/29517688/lib/content/elemHideEmulation.js#newcode479 lib/content/elemHideEmulation.js:479: exports.splitSelector = splitSelector; do we still need this since ...
2 years, 6 months ago (2017-08-16 16:14:47 UTC) #2
Wladimir Palant
The initial idea was that chromium_process.js would be a generic helper whereas test_runner.js would contain ...
2 years, 6 months ago (2017-08-17 10:05:37 UTC) #3
Wladimir Palant
https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js#newcode293 chromium_process.js:293: }); Actually, this shouldn't be necessary any more. Running ...
2 years, 6 months ago (2017-08-17 10:07:31 UTC) #4
kzar
Patch Set 2 : Addressed Wladimir's and Hubert's initial feedback https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js File chromium_process.js (right): https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js#newcode32 ...
2 years, 6 months ago (2017-08-17 12:40:22 UTC) #5
kzar
Patch Set 3 : runBrowserTests should always return a Promise
2 years, 6 months ago (2017-08-17 12:59:50 UTC) #6
Wladimir Palant
https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js File test_runner.js (right): https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newcode27 test_runner.js:27: const MemoryFS = require("memory-fs"); Nit: sort the imports alphabetically? ...
2 years, 6 months ago (2017-08-17 13:06:11 UTC) #7
kzar
Patch Set 4 : Addressed nits https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js File test_runner.js (right): https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newcode27 test_runner.js:27: const MemoryFS = ...
2 years, 6 months ago (2017-08-17 13:26:44 UTC) #8
Wladimir Palant
LGTM with the nit below addressed. https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js File test_runner.js (right): https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newcode27 test_runner.js:27: const MemoryFS = ...
2 years, 6 months ago (2017-08-18 20:49:57 UTC) #9
hub
LGTM (with the same nit) I'll let you land this first so I can rebase ...
2 years, 6 months ago (2017-08-18 20:52:36 UTC) #10
kzar
2 years, 6 months ago (2017-08-21 09:07:01 UTC) #11
https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:27: const MemoryFS = require("memory-fs");
On 2017/08/18 20:49:56, Wladimir Palant wrote:
> On 2017/08/17 13:26:42, kzar wrote:
> > Well I did but case sensitively, I considered A to fall after z.
> 
> Actually, people usually sort by module name, not by the names of imported
> symbols ;)

Done.
Sign in to reply to this message.

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