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

Issue 29354864: Issue 4223 - Migrate some more of adblockplustests (Closed)

Created:
Sept. 25, 2016, 1:38 p.m. by kzar
Modified:
Oct. 4, 2016, 12:32 p.m.
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

Issue 4223 - Migrate some more of adblockplustests

Patch Set 1 #

Total comments: 11

Patch Set 2 : Use sandboxed-module #

Total comments: 1

Patch Set 3 : Get filterListener tests passing reliably #

Patch Set 4 : Added filterNotifier tests, improved sourceTransformer logic #

Total comments: 1

Patch Set 5 : Migrate filterStorage tests #

Total comments: 32

Patch Set 6 : Addressed feedback #

Patch Set 7 : Moved test/stub-modules/common.js to test/common.js #

Patch Set 8 : Removed NODE_PATH env variable #

Patch Set 9 : Allow tests to be run from different directory #

Patch Set 10 : Fixed typo in test_runner.js #

Total comments: 16

Patch Set 11 : Addressed feedback #

Total comments: 2

Patch Set 12 : Addressed final nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2178 lines, -37 lines) Patch
M package.json View 1 2 3 4 5 6 7 1 chunk +2 lines, -2 lines 0 comments Download
A test/_common.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +104 lines, -0 lines 0 comments Download
A test/cssRules.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +136 lines, -0 lines 0 comments Download
A test/domainRestrictions.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +214 lines, -0 lines 0 comments Download
M test/elemHide.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +14 lines, -8 lines 0 comments Download
M test/filterClasses.js View 1 2 3 4 5 6 7 8 9 10 2 chunks +31 lines, -8 lines 0 comments Download
A test/filterListener.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +327 lines, -0 lines 0 comments Download
A test/filterNotifier.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +107 lines, -0 lines 0 comments Download
A test/filterStorage.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +500 lines, -0 lines 0 comments Download
A test/matcher.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +248 lines, -0 lines 0 comments Download
A test/regexpFilters_matching.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +363 lines, -0 lines 0 comments Download
M test/signatures.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +13 lines, -1 line 0 comments Download
M test/stub-modules/io.js View 1 chunk +43 lines, -1 line 0 comments Download
M test/stub-modules/prefs.js View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M test/subscriptionClasses.js View 1 2 3 4 5 6 7 8 9 10 2 chunks +22 lines, -7 lines 0 comments Download
A test_runner.js View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +51 lines, -0 lines 0 comments Download
M test_wrapper.js View 1 1 chunk +0 lines, -9 lines 0 comments Download

Messages

Total messages: 17
kzar
Patch Set 1 https://codereview.adblockplus.org/29354864/diff/29354865/lib/cssRules.js File lib/cssRules.js (right): https://codereview.adblockplus.org/29354864/diff/29354865/lib/cssRules.js#newcode77 lib/cssRules.js:77: _getAllRules: function() (Not to happy with ...
Sept. 25, 2016, 1:49 p.m. (2016-09-25 13:49:39 UTC) #1
Wladimir Palant
https://codereview.adblockplus.org/29354864/diff/29354865/lib/cssRules.js File lib/cssRules.js (right): https://codereview.adblockplus.org/29354864/diff/29354865/lib/cssRules.js#newcode77 lib/cssRules.js:77: _getAllRules: function() On 2016/09/25 13:49:39, kzar wrote: > (Not ...
Sept. 26, 2016, 9:40 a.m. (2016-09-26 09:40:58 UTC) #2
kzar
Patch Set 2 : Use sandboxed-module https://codereview.adblockplus.org/29354864/diff/29354865/test/cssRules.js File test/cssRules.js (right): https://codereview.adblockplus.org/29354864/diff/29354865/test/cssRules.js#newcode29 test/cssRules.js:29: common.prepareFilterComponents.call(this, true); On ...
Sept. 29, 2016, 3:53 p.m. (2016-09-29 15:53:11 UTC) #3
kzar
Patch Set 3 : Get filterListener tests passing reliably https://codereview.adblockplus.org/29354864/diff/29354865/test/filterListener.js File test/filterListener.js (right): https://codereview.adblockplus.org/29354864/diff/29354865/test/filterListener.js#newcode199 test/filterListener.js:199: ...
Sept. 29, 2016, 3:58 p.m. (2016-09-29 15:58:57 UTC) #4
kzar
Patch Set 4 : Added filterNotifier tests, improved sourceTransformer logic https://codereview.adblockplus.org/29354864/diff/29355314/test/stub-modules/common.js File test/stub-modules/common.js (right): https://codereview.adblockplus.org/29354864/diff/29355314/test/stub-modules/common.js#newcode29 ...
Sept. 29, 2016, 6:19 p.m. (2016-09-29 18:19:38 UTC) #5
kzar
Patch Set 5 : Migrate filterStorage tests
Sept. 29, 2016, 7:24 p.m. (2016-09-29 19:24:57 UTC) #6
Wladimir Palant
https://codereview.adblockplus.org/29354864/diff/29355316/package.json File package.json (right): https://codereview.adblockplus.org/29354864/diff/29355316/package.json#newcode11 package.json:11: "test": "NODE_PATH=lib:test/stub-modules node -e 'require(\".bin/nodeunit\");'" Why not just: NODE_PATH=lib:test/stub-modules ...
Sept. 30, 2016, 9:37 a.m. (2016-09-30 09:37:56 UTC) #7
kzar
Patch Set 6 : Addressed feedback https://codereview.adblockplus.org/29354864/diff/29355316/package.json File package.json (right): https://codereview.adblockplus.org/29354864/diff/29355316/package.json#newcode11 package.json:11: "test": "NODE_PATH=lib:test/stub-modules node ...
Oct. 3, 2016, 1:46 p.m. (2016-10-03 13:46:17 UTC) #8
kzar
Patch Set 7 : Moved test/stub-modules/common.js to test/common.js https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/common.js File test/stub-modules/common.js (right): https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/common.js#newcode1 test/stub-modules/common.js:1: let ...
Oct. 3, 2016, 4:43 p.m. (2016-10-03 16:43:15 UTC) #9
kzar
Patch Set 8 : Removed NODE_PATH env variable https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/common.js File test/stub-modules/common.js (right): https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/common.js#newcode31 test/stub-modules/common.js:31: let ...
Oct. 3, 2016, 5:47 p.m. (2016-10-03 17:47:43 UTC) #10
kzar
Patch Set 9 : Allow tests to be run from different directory
Oct. 3, 2016, 6:02 p.m. (2016-10-03 18:02:11 UTC) #11
kzar
Patch Set 10 : Fixed typo in test_runner.js
Oct. 4, 2016, 9:44 a.m. (2016-10-04 09:44:07 UTC) #12
Wladimir Palant
https://codereview.adblockplus.org/29354864/diff/29355623/test/common.js File test/common.js (right): https://codereview.adblockplus.org/29354864/diff/29355623/test/common.js#newcode68 test/common.js:68: for (let name of extraExports) Wrong indentation here, single ...
Oct. 4, 2016, 11:03 a.m. (2016-10-04 11:03:00 UTC) #13
kzar
Patch Set 11 : Addressed feedback https://codereview.adblockplus.org/29354864/diff/29355623/test/common.js File test/common.js (right): https://codereview.adblockplus.org/29354864/diff/29355623/test/common.js#newcode68 test/common.js:68: for (let name ...
Oct. 4, 2016, 12:06 p.m. (2016-10-04 12:06:25 UTC) #14
Wladimir Palant
LGTM but feel free to address the nit below before pushing. Nice, it looks like ...
Oct. 4, 2016, 12:15 p.m. (2016-10-04 12:15:06 UTC) #15
kzar
Patch Set 12 : Addressed final nit https://codereview.adblockplus.org/29354864/diff/29355641/test_runner.js File test_runner.js (right): https://codereview.adblockplus.org/29354864/diff/29355641/test_runner.js#newcode40 test_runner.js:40: path.basename(testPath)[0] != ...
Oct. 4, 2016, 12:17 p.m. (2016-10-04 12:17:27 UTC) #16
Wladimir Palant
Oct. 4, 2016, 12:26 p.m. (2016-10-04 12:26:27 UTC) #17
Even more LGTM.

Powered by Google App Engine
This is Rietveld