Ensure that tests-affected picks up changes to webidl/idlharness #8069
Conversation
Build BROKENStarted: 2017-11-06 00:22:03 Failing Jobs
View more information about this build on: |
|
Typo: reources |
|
Typo: seperator |
tools/wpt/testfiles.py
Outdated
| def files_changed(revish, ignore_rules=None, include_uncommitted=False, include_new=False): | ||
| """Get and return files changed since current branch diverged from master, | ||
| excluding those that are located within any directory specifed by | ||
| `ignore_changes`.""" |
There was a problem hiding this comment.
There isn't anything called ignore_changes.
| parts = rule.split("/") | ||
| re_parts = [] | ||
| for part in parts: | ||
| if part.endswith("**"): |
There was a problem hiding this comment.
Parsing out what the body of this loop does isn't easy, for me. Maybe a test for this that hits each of the branches? (Don't need to apply the rules, just stringify the resulting regexes and asssert that they are what they should be.)
There was a problem hiding this comment.
Tests now in test_files_changed_ignore_rules()
tools/wpt/tests/test_wpt.py
Outdated
| @@ -91,6 +91,14 @@ def test_files_changed(capsys): | |||
| assert err == "" | |||
|
|
|||
|
|
|||
| def test_files_changed_ignore(capsys): | |||
| from wpt.testfiles import exclude_ignored | |||
| files = ["resources/testharness.js", "resources/webidl2/index.js", "test/test.js"] | |||
There was a problem hiding this comment.
Having webidl2.js show up in a test would be nice, since that was the thing that originally didn't work.
There was a problem hiding this comment.
That's not entirely trivial to test since it only affects tests_affected rather than get_files_changed and that's mostly about reading files to see what data they contain, so it would either need to be refactored or we'd need to find some commit with a webidl2 change to test, but there aren't many of those. Maybe land the upgrade and use that commit for the test.
There was a problem hiding this comment.
Would a previous commit that changes webidl2.js do the job? 960a3d2?
jgraham
force-pushed
the
idl_affected
branch
2 times, most recently
from
aa8c0ff
to
28b1265
Compare
When checking the tests that are affected by a change we previously had a rule to ignore all files under resources, so that changes to testharness.js don't cause the entire repository to be rerun. However changes to WebIDL2 or idlharness.js probably should cause all interfaces.html tests to be rerun. That requires two changes: * Support for the rewrite rule that causes /resources/WebIDLParser.js to be interpreted as the resources/webidl2/lib/webidl2.js path * Support for more precise exclude rules so that we can exclude resources/testharness.js without excluding all of the WebIDL stuff. For the latter --ignore-dirs has been replaced by --ignore-rules, where the rules can include a * to match anything other than a path separator or ** to match anything. In either case the patterns are only valid at the end of a path segment (i.e. before a separator or the end of the path). Further changes may be required if we want to allow changes to webidl2 that don't affect the webidl2.js file itself to be seen as affecting all idlharness tests.
|
|
||
| def test_files_changed_ignore_rules(): | ||
| from tools.wpt.testfiles import compile_ignore_rule | ||
| assert compile_ignore_rule("foo*bar*/baz").pattern == "^foo\*bar[^/]*/baz$" |
There was a problem hiding this comment.
Is this really right? Why is \* part of the pattern here or anywhere, and not .*?
There was a problem hiding this comment.
It's just doing prefix matching on path parts. It could do something different, but this is enough to satisfy the current use cases.
There was a problem hiding this comment.
Oh, the comment "* before a separator or the end matching" explains this.
| parts = rule.split("/") | ||
| re_parts = [] | ||
| for part in parts: | ||
| if part.endswith("**"): |
There was a problem hiding this comment.
Tests now in test_files_changed_ignore_rules()
When checking the tests that are affected by a change we previously
had a rule to ignore all files under reources, so that changes to
testharness.js don't cause the entire repository to be rerun. However
changes to WebIDL2 or idlharness.js probably should cause all
interfaces.html tests to be rerun. That requires two changes:
Support for the rewrite rule that causes /resources/WebIDLParser.js
to be interpreted as the resources/webidl2/lib/webidl2.js path
Support for more precise exclude rules so that we can exclude
resources/testharness.js without excluding all of the WebIDL stuff.
For the latter --ignore-dirs has been replaced by --ignore-rules,
where the rules can include a * to match anything other than a path
separator or ** to match anything. In either case the patterns are
only valid at the end of a path segment (i.e. before a seperator or
the end of the path).
Further changes may be required if we want to allow changes to webidl2
that don't affect the webidl2.js file itself to be seen as affecting
all idlharness tests.
This change is