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

Issue 6612283790721024: Issue 698 - Added tests for $sitekey option (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years ago by Thomas Greiner
Modified:
4 years, 9 months ago
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

Issue 698 - Added tests for $sitekey option

Patch Set 1 #

Total comments: 20

Patch Set 2 : #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+508 lines, -312 lines) Patch
M chrome/content/tests/filterClasses.js View 1 3 chunks +6 lines, -2 lines 0 comments Download
M chrome/content/tests/matcher.js View 1 3 chunks +69 lines, -61 lines 0 comments Download
M chrome/content/tests/policy.js View 1 8 chunks +37 lines, -2 lines 0 comments Download
M chrome/content/tests/regexpFilters_matching.js View 1 1 chunk +266 lines, -247 lines 1 comment Download
A chrome/content/tests/signatures.js View 1 chunk +130 lines, -0 lines 0 comments Download

Messages

Total messages: 6
Thomas Greiner
5 years ago (2014-09-02 14:08:04 UTC) #1
Wladimir Palant
http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chrome/content/tests/filterClasses.js File chrome/content/tests/filterClasses.js (right): http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chrome/content/tests/filterClasses.js#newcode42 chrome/content/tests/filterClasses.js:42: result.push("sitekeys=" + sitekeys.sort().join("|")); sitekeys.sort() will have side-effects - it ...
5 years ago (2014-09-10 21:55:06 UTC) #2
Wladimir Palant
May I remind you that this issue is still outstanding?
4 years, 9 months ago (2014-12-09 21:37:29 UTC) #3
Thomas Greiner
On 2014/12/09 21:37:29, Wladimir Palant wrote: > May I remind you that this issue is ...
4 years, 9 months ago (2014-12-11 11:26:47 UTC) #4
Thomas Greiner
http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chrome/content/tests/filterClasses.js File chrome/content/tests/filterClasses.js (right): http://codereview.adblockplus.org/6612283790721024/diff/5629499534213120/chrome/content/tests/filterClasses.js#newcode42 chrome/content/tests/filterClasses.js:42: result.push("sitekeys=" + sitekeys.sort().join("|")); On 2014/09/10 21:55:06, Wladimir Palant wrote: ...
4 years, 9 months ago (2014-12-11 14:33:55 UTC) #5
Wladimir Palant
4 years, 9 months ago (2014-12-11 15:57:47 UTC) #6
LGTM but feel free to address the nit below.

http://codereview.adblockplus.org/6612283790721024/diff/5700735861784576/chro...
File chrome/content/tests/regexpFilters_matching.js (right):

http://codereview.adblockplus.org/6612283790721024/diff/5700735861784576/chro...
chrome/content/tests/regexpFilters_matching.js:292:
testMatch("abc$domain=bar.com,sitekey=foo-publickey", "http://abc/def", "IMAGE",
"bar.com", true, "foo-publickey", true);
Nit: the last two tests are redundant, they test the scenario "Domain specified
in filter along with sitekey and matches/doesn't match document domain" - this
was tested already, merely with foo.com and bar.com reversed.
Sign in to reply to this message.

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