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

Conversation

@fuglede
Copy link
Contributor

@fuglede fuglede commented Oct 19, 2015

This adds a simple shell script which runs all changed rulesets by https-everywhere-checker.

It happens every now and then that people submit rulesets that fail only because of subtle SSL misconfigurations (typically broken certificate chains), and this helps to catch those.

There are some open questions here, including how to handle folder/file names (commit.sh seems a bit weird given its concrete purpose) and how to include Travis, if at all. A complementary option here would be to run the test only on Travis and not locally.

This PR presumes jsha/https-everywhere-checker#4 (which is what causes Travis to fail).

Edit: Ah, hm, I just realised that this does more or less the same thing as hooks/precommit. They could probably be merged.

@fuglede fuglede changed the title Add pre-commit hook with unit test for ruleset test URLs Add pre-commit hook with test for ruleset test URLs Oct 19, 2015
This adds a simple shell script which runs all changed rulesets by https-everywhere-checker.

It happens every now and then that people submit rulesets that fail only because of subtle SSL misconfigurations (typically broken certificate chains), and this helps to catch those.
@jsha
Copy link
Member

jsha commented Dec 18, 2015

This looks nice, in particular having Travis do a fetch test of only the changed rules.

I'd prefer not to auto-add it as a commit hook, because the test makes external HTTP requests (vs the existing rules check which is purely local). I think some of our developers may be sensitive about such requests, so we should make the fetch test opt-in.

@fuglede
Copy link
Contributor Author

fuglede commented Jan 18, 2016

Yeah, I see what you are saying, but wouldn't contributors normally have already thrown tons of requests towards the host to be checked? Let's put it in Travis though; I have yet to understand how exactly that will pan out when ruleset changes span multiple commits but I guess there is only one way to find out.

@fuglede fuglede closed this Jan 18, 2016
Following jsha's suggestion in EFForg#3107 (comment), this removes the pre-commit test from the developer installation script, but adds it instead to Travis.
@fuglede fuglede reopened this Jan 18, 2016
@fuglede
Copy link
Contributor Author

fuglede commented Jan 18, 2016

Would there be an obvious way to have Travis figure out what rulesets have changed. The git diff --name-only HEAD tricks does not appear to work: this build included an intentionally broken ruleset, yet the test runs fine on Travis;

$ test/commit.sh

The command "test/commit.sh" exited with 0.

@fuglede
Copy link
Contributor Author

fuglede commented Jan 18, 2016

Ah, maybe I am overthinking this, but fetching the current master and comparing a PR to that solves this in a commit-independent fashion; here's an example of a breaking build:

$ test/commit.sh

From https://github.com/fuglede/https-everywhere

 * branch            master     -> FETCH_HEAD

 * [new branch]      master     -> upstream-for-travis/master

Ruleset database has changed. Testing test URLs in all changed rulesets.

INFO Finished comparing http://multimedia.pol.dk/ -> https://multimedia.pol.dk/. Rulefile: src/chrome/content/rules/Pol.dk.xml.

ERROR src/chrome/content/rules/Pol.dk.xml: Fetch error: http://multimedia.pol.dk/ => https://multimedia.pol.dk/: (7, 'Failed to connect to multimedia.pol.dk port 443: Connection refused')

INFO Finished in 0.39 seconds. Loaded rulesets: 1, URL pairs: 1.

Test URL test failed.

The command "test/commit.sh" exited with 1.

I apologize for being spammy (again).

This renames the pre-commit test to reflect that it is now Travis specific; rather than the contents of the previous commit, this checks the current branch against the EFForg GitHub repository master branch.
@fuglede fuglede changed the title Add pre-commit hook with test for ruleset test URLs Add Travis test for ruleset test URLs Feb 2, 2016
This was referenced Mar 6, 2016
@J0WI
Copy link
Contributor

J0WI commented Mar 17, 2016

This would still be really helpful. :)

@jsha
Copy link
Member

jsha commented Mar 17, 2016

Here's how we achieve this in Boulder-land, which has worked fairly well:

# Travis does shallow clones, so there is no master branch present.                                                                                                                                                                           
# But test-no-outdated-migrations.sh needs to check diffs against master.                                                                                                                                                                     
# Fetch just the master branch from origin.                                                                                                                                                                                                   
( git fetch origin master                                                                                                                                                                                                                     
git branch master FETCH_HEAD ) &   

Then elsewhere:

git diff --name-only master -- ${DB_DIR} > ${NEW_FILES_LIST}

I think the same approach could be applied here. I agree it would be really helpful!

@fuglede
Copy link
Contributor Author

fuglede commented Mar 18, 2016

Thanks for checking it out again @jsha; I think that's actually equivalent to what I ended up with?

@fuglede
Copy link
Contributor Author

fuglede commented Mar 24, 2016

I guess this one is outdated now that https-everywhere-checker has been moved to the main repo. Shouldn't the existing issues have moved alongside it for the sake of reference? (cc @Hainish)

@Hainish
Copy link
Member

Hainish commented Mar 25, 2016

@fuglede only validation tests are run via CI, so fetch tests on only the changed rules are still useful. I've created an issue to migrate the checker issues: #4476

@fuglede
Copy link
Contributor Author

fuglede commented Apr 10, 2016

Finally got around to merging current master into this. In particular, with test/rules having been moved from the submodule to the repo itself, this is no longer blocked by jsha/https-everywhere-checker#4, so that's nifty.

@Hainish Hainish merged commit d74afb5 into EFForg:master Apr 11, 2016
@Hainish
Copy link
Member

Hainish commented Apr 11, 2016

Works great, thanks @fuglede !

@fuglede
Copy link
Contributor Author

fuglede commented Apr 12, 2016

Fantastic, thanks for reviewing it! Looks like it's catching fish already.

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