Skip to content
This repository was archived by the owner on Nov 6, 2023. It is now read-only.

Disable Rules Test Update#17000

Merged
Hainish merged 9 commits into
EFForg:masterfrom
zoracon:disable-rules-test-check-update
Nov 26, 2018
Merged

Disable Rules Test Update#17000
Hainish merged 9 commits into
EFForg:masterfrom
zoracon:disable-rules-test-check-update

Conversation

@zoracon

@zoracon zoracon commented Oct 30, 2018

Copy link
Copy Markdown
Contributor

This is directly off of the work done in #11616,
The only thing I added here was to disable the ruleset if all targets turn out to be problematic

@zoracon

zoracon commented Oct 30, 2018

Copy link
Copy Markdown
Contributor Author

@Hainish Requesting review when you get a chance to look it over

@zoracon

zoracon commented Oct 30, 2018

Copy link
Copy Markdown
Contributor Author

I also realize my text editor seemed to reformat this file for some reason :/. So I will list the function i changed here:
https://github.com/EFForg/https-everywhere/pull/17000/files#diff-db13e72ba2cc41fb2fc62d0cc64540c3R216

@Bisaloo

Bisaloo commented Oct 30, 2018

Copy link
Copy Markdown
Collaborator

https://github.com/EFForg/https-everywhere/pull/17000/files?w=1

Diff without the whitespace changes. Just add ?w=1 to the URL 😉

@zoracon

zoracon commented Oct 30, 2018

Copy link
Copy Markdown
Contributor Author

@Bisaloo Thanks for the tip! Didn't know about that trick

@cschanaj cschanaj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, it is better to comment out the failing target instead of removing them completely.

Comment thread test/rules/src/https_everywhere_checker/check_rules.py Outdated
Comment thread test/rules/src/https_everywhere_checker/check_rules.py Outdated
Comment thread test/rules/src/https_everywhere_checker/check_rules.py Outdated

@Hainish Hainish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - just a few python3-friendly changes requested.

Comment thread test/rules/src/https_everywhere_checker/check_rules.py Outdated
Comment thread test/rules/src/https_everywhere_checker/check_rules.py Outdated
Comment thread test/rules/src/https_everywhere_checker/check_rules.py Outdated
Comment thread test/rules/src/https_everywhere_checker/check_rules.py Outdated
Comment thread test/rules/src/https_everywhere_checker/check_rules.py Outdated
Chan Chak Shing and others added 4 commits October 31, 2018 13:27
Co-Authored-By: zoracon <lxshancock@gmail.com>
Co-Authored-By: zoracon <lxshancock@gmail.com>
Co-Authored-By: zoracon <lxshancock@gmail.com>
@zoracon

zoracon commented Nov 7, 2018

Copy link
Copy Markdown
Contributor Author

@Hainish Applied the format changes for more up to date conventions, asking for one more once over before merge

@Bisaloo

Bisaloo commented Nov 8, 2018

Copy link
Copy Markdown
Collaborator

Fixes #1998

@cschanaj

cschanaj commented Nov 9, 2018

Copy link
Copy Markdown
Collaborator

AFAIK, full-fetch-test will re-activate previously disabled rulesets on subsequent runs. It might be even better if we could use comment patterns which can be easily reverted, e.g.

<!-- target host="example.com" **/*-->

See https://github.com/EFForg/https-everywhere-full-fetch-test/blob/4ca85e146c86fd35c7810fb763360750f6085fe3/test.sh#L4

@Hainish Hainish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once these two issues are resolved I'm ready to merge.

Comment thread test/rules/src/https_everywhere_checker/check_rules.py Outdated
Comment thread test/rules/src/https_everywhere_checker/check_rules.py Outdated
Comment thread test/rules/src/https_everywhere_checker/check_rules.py Outdated
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants