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

Issue 29342884: Issue 4055 - Test ElemHide.getSelectorsFordomain (Closed)

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

Description

Issue 4055 - Test ElemHide.getSelectorsFordomain

Patch Set 1 #

Total comments: 1

Patch Set 2 : Add test and note about known limitation. #

Patch Set 3 : Ignore selector order #

Total comments: 14

Patch Set 4 : Addressed feedback #

Total comments: 2

Patch Set 5 : Properly test and document duplicate selector behaviour #

Total comments: 4

Patch Set 6 : Simplify filter function and move comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+174 lines, -1 line) Patch
M package.json View 2 chunks +2 lines, -1 line 0 comments Download
A test/elemHide.js View 1 2 3 4 5 1 chunk +169 lines, -0 lines 0 comments Download
A test/stub-modules/io.js View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A test/stub-modules/prefs.js View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A test/stub-modules/utils.js View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 10
kzar
Patch Set 1 https://codereview.adblockplus.org/29342884/diff/29342885/package.json File package.json (right): https://codereview.adblockplus.org/29342884/diff/29342885/package.json#newcode3 package.json:3: "repository": "https://hg.adblockplus.org/adblockpluscore", This stops NPM complaining.
3 years, 6 months ago (2016-05-22 16:43:40 UTC) #1
kzar
Patch Set 2 : Add test and note about known limitation.
3 years, 6 months ago (2016-05-22 20:31:27 UTC) #2
kzar
Patch Set 3 : Ignore selector order https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#newcode39 test/elemHide.js:39: test.deepEqual(ElemHide.getSelectorsForDomain(domain, specificOnly).sort(), ...
3 years, 6 months ago (2016-05-23 11:46:50 UTC) #3
Wladimir Palant
https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#newcode20 test/elemHide.js:20: GLOBAL.Ci = { }; Nit: We usually write that ...
3 years, 6 months ago (2016-05-23 13:23:02 UTC) #4
kzar
Patch Set 4 : Addressed feedback https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#newcode20 test/elemHide.js:20: GLOBAL.Ci = { ...
3 years, 6 months ago (2016-05-23 13:47:52 UTC) #5
Wladimir Palant
https://codereview.adblockplus.org/29342884/diff/29342925/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29342884/diff/29342925/test/elemHide.js#newcode138 test/elemHide.js:138: selectorsEqual("example.com", ["foo", "foo"]); Here you are cementing what is ...
3 years, 6 months ago (2016-05-23 15:06:47 UTC) #6
kzar
Patch Set 5 : Properly test and document duplicate selector behaviour https://codereview.adblockplus.org/29342884/diff/29342925/test/elemHide.js File test/elemHide.js (right): ...
3 years, 6 months ago (2016-05-23 15:49:20 UTC) #7
Wladimir Palant
https://codereview.adblockplus.org/29342884/diff/29342937/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29342884/diff/29342937/test/elemHide.js#newcode47 test/elemHide.js:47: }); How about a simpler filter function? (selector, index, ...
3 years, 6 months ago (2016-05-23 17:34:48 UTC) #8
kzar
Patch Set 6 : Simplify filter function and move comment https://codereview.adblockplus.org/29342884/diff/29342937/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29342884/diff/29342937/test/elemHide.js#newcode47 ...
3 years, 6 months ago (2016-05-23 18:56:27 UTC) #9
Wladimir Palant
3 years, 6 months ago (2016-05-23 19:01:07 UTC) #10
LGTM
Sign in to reply to this message.

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