Verify each target has the right number of tests #11998

cschanaj · 2017-08-20T06:56:43Z

blocked by #11187 which has been pending review for a long time... Also, merge #11942 before whitelisting rules-test.

See https://gist.github.com/cschanaj/b3c80c935226c55e78013d81d618cd99

Related: #7272

cschanaj · 2017-08-21T11:57:34Z

test/rules/src/https_everywhere_checker/rules.py

+			if not "*" in target and not self.excludes(("http://%s/" % target)):
+				continue
+
+			# '*.example.com' match 'www.example.com' but not 'secure.account.exmple.com'


@Hainish does *.example.com matches www.example.com, secure.account.example.com and example.com? I am a bit confused seeing https://support.google.com/customsearch/answer/71826?hl=en and I not not sure how https-everywhere treat them. thanks in advance!

@cschanaj the logic is here:

https-everywhere/chromium/rules.js

Lines 350 to 355 in 454fb4a

// now eat away from the left, with *, so that for x.y.z.google.com we

// check *.z.google.com and *.google.com (we did *.y.z.google.com above)

for (var i = 2; i <= segmented.length - 2; ++i) {

var t = "*." + segmented.slice(i,segmented.length).join(".");

results = results.concat(this.targets[t]);

}

So to answer, for a.b.examble.com, *.example.com is a matching target.

cschanaj · 2017-08-23T00:59:47Z

@Hainish I have fixed the wildcard matching logic in c255212 and this is ready for an review. thanks!!

P.S. please have a look at #11942 which fix a trivial bug in the ruleset whitelist cleanup script.

Hainish · 2017-08-23T01:42:16Z

Got it, I should have some time in the next few days to review this.

cschanaj · 2017-08-24T02:11:23Z

@Hainish It seems that we treat * in different position differently, according to my understanding, it seem that

implementation in `rules.js`

`target`	`regex` interpretation
`*.example.com`	`^.+\.example\.com$`
`secure.*.example.com`	`^secure\.[^\.]+\.example\.com$`
`www.example.*`	`^www\.example\.[^\.]+$`

~~I will have to make changes to this PR if this is true, please confirm.~~ thanks!

cschanaj · 2017-08-25T16:06:47Z

@Hainish merge #12091 will fix Travis from this PR. thanks!

cowlicks · 2017-09-11T18:09:15Z

@cschanaj I'm looking into this today! Sorry for the delay.

cowlicks · 2017-09-11T18:57:20Z

test/rules/src/https_everywhere_checker/rules.py

+				continue
+
+			# According to the logic in rules.js available at
+			# EFForg/https-everywhere/blob/master/chromium/rules.js#L350-L355


Can you include the commit hash in this URL? This way we can find the referenced code if it moves. You can get a permalink to lines in a specific version of the file by selecting the lines normally, then pressing "y".

cowlicks · 2017-09-11T19:08:44Z

test/rules/src/https_everywhere_checker/rules.py

+				pattern = pattern.replace('*', '.+')
+
+			# however, `example.*` match `example.com` but not `example.co.uk`
+			if pattern[len(pattern) - 1] == '*':


You can get the last element like this: pattern[-1]

cowlicks · 2017-09-12T02:12:04Z

@cschanaj I'm not very familiar with running these tests. Would you mind providing a testcase this PR fixes? I would just run it by pointing the rulesdir in checker.config to it, right?

cschanaj · 2017-09-13T11:13:39Z

@cowlicks Sadly, Travis does not trigger rules test if there is only core code changes. I run the rules test by the following command

$ export TEST=rules
$ ./test.sh

cowlicks

One bug, should be simple to fix. Everything else seems fine.

cowlicks · 2017-09-13T21:13:18Z

test/rules/src/https_everywhere_checker/rules.py

-		"""Verify that each rule and each exclusion has the right number of tests
-			 that applies to it. TODO: Also check that each target has the right
+		"""Verify that each target, rule and exclusion has the right number of tests
+			 that applies to it. Also check that each target has the right
 			 number of tests. In particular left-wildcard targets should have at least
 			 three tests. Right-wildcard targets should have at least ten tests.

 			 Returns an array of strings reporting any coverage problems if they exist,
 			 or empty list if coverage is sufficient.
 			 """
 		problems = self._determineTestApplication()


Every "problem" gets printed twice, they are created here in getCoverageProblems. This happens because problems is a reference to self.test_application_problems through self._determineTestApplication. Then again in getTargetValidityProblems we do problems = self._determineTestApplication, so the problems from getCoverageProblems also get reported through getTargetValidityProblems.

I'm not sure why problems is assigned like this. I'm guessing it is a mistake in either this function or getTargetValidityProblems. Maybe @Hainish can chime in.

@cschanaj You can probably just change this to:

self._determineTestApplication() problems = []

that is what other tests do

cowlicks · 2017-09-19T19:41:45Z

@cschanaj git can be tricky sometimes. I can fix this for you if you'd like.

cowlicks · 2017-10-18T01:04:38Z

I revisited this today. It looks good to me. It should be merged in conjunction with #1209, but #12091 needs to be updated again. Can you update it @cschanaj ? then we can merge this.

Also, docker on travis is failing for some reason.

cschanaj · 2017-10-18T11:50:38Z

@cowlicks I have rebased this and merged #12091 here. I will update #11560 once this is merged. thanks!

cowlicks · 2017-10-18T18:12:41Z

@cschanaj great work. Thank you.

* Remove outdated TODO (fixed by #11998) * Remove html encoded characters * Surround XML tags and code with backticks * Wrap source to 80 characters * Add missing slash

cschanaj mentioned this pull request Aug 20, 2017

Insufficient test coverage error not triggered by wildcard target and no tests #7272

Closed

J0WI added the ruleset-testing label Aug 20, 2017

cschanaj commented Aug 21, 2017

View reviewed changes

cschanaj mentioned this pull request Aug 23, 2017

Fix Traivs rules-test from #11998 #12057

Closed

Hainish assigned cowlicks Aug 31, 2017

cschanaj mentioned this pull request Sep 2, 2017

Unnecessary overlapping targets in rulesets #12322

Closed

cowlicks reviewed Sep 11, 2017

View reviewed changes

cowlicks reviewed Sep 13, 2017

View reviewed changes

cschanaj mentioned this pull request Sep 19, 2017

Create utils/full-rules-test.sh #11561

Closed

cschanaj mentioned this pull request Oct 18, 2017

Fix Travis rules-test from #11998 (Revised) #12091

Closed

Pasu Chan and others added 7 commits October 18, 2017 19:27

Verify each target has the right number of tests

14b0e0b

Fix wildcard matching logic

1109444

Fix Logic for right-wildcards

bb042ce

Fix Logic for middle wildcards

2e206c5

Update https_everywhere_checker/rules.py

44a11df

Fix a small bug which every problem get printed twice

6dcd289

Whitelist rulesets failing rules-test

d980b8b

cowlicks merged commit 32e9cac into EFForg:master Oct 18, 2017

cschanaj deleted the check-test branch October 18, 2017 22:26

cschanaj mentioned this pull request Oct 19, 2017

Remove false-positive from ruleset-whitelist.csv (full-rules-test) #11560

Closed

Bisaloo mentioned this pull request Oct 24, 2017

Update ruleset-whitelist.csv for rulesets without sufficient coverage #13322

Merged

J0WI pushed a commit that referenced this pull request Dec 1, 2017

Update and clean up ruleset-testing.md (#13551)

b8e657c

* Remove outdated TODO (fixed by #11998) * Remove html encoded characters * Surround XML tags and code with backticks * Wrap source to 80 characters * Add missing slash

Bisaloo mentioned this pull request Jan 13, 2018

Require minimum test URLs per target host #4477

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify each target has the right number of tests #11998

Verify each target has the right number of tests #11998

cschanaj commented Aug 20, 2017 •

edited

Loading

cschanaj Aug 21, 2017 •

edited

Loading

Hainish Aug 22, 2017 •

edited

Loading

cschanaj commented Aug 23, 2017

Hainish commented Aug 23, 2017

cschanaj commented Aug 24, 2017 •

edited

Loading

cschanaj commented Aug 25, 2017

cowlicks commented Sep 11, 2017

cowlicks Sep 11, 2017

cowlicks Sep 11, 2017

cowlicks commented Sep 12, 2017

cschanaj commented Sep 13, 2017 •

edited

Loading

cowlicks left a comment

cowlicks Sep 13, 2017

cowlicks Sep 14, 2017

cowlicks commented Sep 19, 2017

cowlicks commented Oct 18, 2017

cschanaj commented Oct 18, 2017

cowlicks commented Oct 18, 2017

	// now eat away from the left, with *, so that for x.y.z.google.com we
	// check .z.google.com and .google.com (we did *.y.z.google.com above)
	for (var i = 2; i <= segmented.length - 2; ++i) {
	var t = "*." + segmented.slice(i,segmented.length).join(".");
	results = results.concat(this.targets[t]);
	}

Verify each target has the right number of tests #11998

Verify each target has the right number of tests #11998

Conversation

cschanaj commented Aug 20, 2017 • edited Loading

cschanaj Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Hainish Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

cschanaj commented Aug 23, 2017

Hainish commented Aug 23, 2017

cschanaj commented Aug 24, 2017 • edited Loading

implementation in rules.js

cschanaj commented Aug 25, 2017

cowlicks commented Sep 11, 2017

cowlicks Sep 11, 2017

Choose a reason for hiding this comment

cowlicks Sep 11, 2017

Choose a reason for hiding this comment

cowlicks commented Sep 12, 2017

cschanaj commented Sep 13, 2017 • edited Loading

cowlicks left a comment

Choose a reason for hiding this comment

cowlicks Sep 13, 2017

Choose a reason for hiding this comment

cowlicks Sep 14, 2017

Choose a reason for hiding this comment

cowlicks commented Sep 19, 2017

cowlicks commented Oct 18, 2017

cschanaj commented Oct 18, 2017

cowlicks commented Oct 18, 2017

cschanaj commented Aug 20, 2017 •

edited

Loading

cschanaj Aug 21, 2017 •

edited

Loading

Hainish Aug 22, 2017 •

edited

Loading

cschanaj commented Aug 24, 2017 •

edited

Loading

implementation in `rules.js`

cschanaj commented Sep 13, 2017 •

edited

Loading