Add tests against non-match groups being used in replacements

[RReverser]

As told on the email, I suggest to add check that ensures that $1..$9 is absent in rewritten URL when going through <test url />s and auto-generated URLs from hosts. This will prevent mistakes such as #4326, #4302, #4301, #4300, #4299, #2980 and similar.

/cc @jsha

[jsha]

@Hainish, can you work on this one?

[Hainish]

Yes, I can start this tomorrow.
On Mar 13, 2016 4:03 PM, "Jacob Hoffman-Andrews" notifications@github.com
wrote:

@Hainish https://github.com/Hainish, can you work on this one?

—
Reply to this email directly or view it on GitHub
#4327 (comment)
.

[Hainish]

@RReverser fetch-test.sh already indicates an error when run on files containing non-match groups in replacements:

 $ ./fetch-test.sh rules/Waffles.fm.xml 
ERROR rules/Waffles.fm.xml: No rule or exclusion applies to test URL http://waffles.fm/

INFO Finished comparing http://waffles.fm/ -> http://waffles.fm/. Rulefile: rules/Waffles.fm.xml.
ERROR rules/Waffles.fm.xml: Regex Error invalid group reference at position 13 ^http://www\.waffles\.fm/
INFO Finished in 2.45 seconds. Loaded rulesets: 1, URL pairs: 2.

Just to be clear, this ticket is for adding it to trivial-validate.py?

[RReverser]

Just to be clear, this ticket is for adding it to trivial-validate.py?

I don't really know codebase structure of HTTPS Everywhere, sorry.

Just wondering - if this test is already present, how come those errors are not produced / caught by CI?

[Hainish]

@RReverser fetch-test.sh is not run by CI because it actually goes and fetches the remote endpoints to check for non-200 responses upon redirect. This would take a long time for every rule. We ordinarily run it on rules that have changed manually.

[RReverser]

@Hainish Oh, but this check doesn't really need to fetch remote endpoints, so maybe makes sense to move it?

[Hainish]

@jsha and I discussed it. This test probably belongs in https_everywhere_checker and should be written as part of a larger rewrite which eliminates the sqlite db, where the tests source directly from the xml files.

[fuglede]

For the sake of reference, #3107 suggests adding a https-everywhere-checker test to the CI.