Closed
Bug 1337674
Opened 8 years ago
Closed 8 years ago
stylo: Land the list of known failures in style system mochitests in-tree and make mochitest-style green
Categories
(Testing :: Mochitest, defect)
Tracking
(firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jmaher
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Although we still have ~30k (probably just 25k after the next merge) failures, we still have >300k passing. We probably want to start doing something to ensure that we don't regress what we have already passed.
I have had a detailed list about the reason of each failure. [1] We can probably land that list somehow, and mark the test green if all failures are expected.
I haven't figured out what is the best way to land this list. Potential solutions include:
1. Just do the coarse-grained thing that marks tests with any failure with "fail-if = stylo". This is easiest, but some of the mochitests include tons of different failures which we may want to detect separately.
2. Add another manifest file which records the failure patterns and counts, and pass this information to test runner, then make SimpleTest._logResult reports TEST-KNOWN-FAIL for them, and maintain a count for each pattern. Finally, log an unexpected failure in SimpleTest.finish() if there is any number mismatched.
3. Add an extra program to filter the result log, and only output patterns whose number doesn't match, and output log items which do not match any known pattern.
The third way is basically what I'm doing locally. But fitting it into the automation may take more effort than second I guess? I feel that the second approach might be the simplest?
[1] https://gist.github.com/upsuper/082268f59e6f267d5a466c2cbe698e58
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → xidorn+moz
Assignee | ||
Comment 1•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8837496 [details]
Bug 1337674 part 1 - Adjust test_load_events_on_stylesheets to not double finish even with error.
https://reviewboard.mozilla.org/r/112608/#review114086
Attachment #8837496 -
Flags: review?(cam) → review+
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8837497 [details]
Bug 1337674 part 2 - Disable style system tests written with testharness.js.
https://reviewboard.mozilla.org/r/112610/#review114088
Attachment #8837497 -
Flags: review?(cam) → review+
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8837499 [details]
Bug 1337674 part 4 - Disable several tests which crash or timeout.
https://reviewboard.mozilla.org/r/112614/#review114090
Attachment #8837499 -
Flags: review?(cam) → review+
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8837498 [details]
Bug 1337674 part 3 - Add failure pattern file support to mochitest.
https://reviewboard.mozilla.org/r/112612/#review114110
really close!
::: testing/mochitest/runtests.py:1447
(Diff revision 1)
> if 'expected' in test:
> testob['expected'] = test['expected']
> if 'scheme' in test:
> testob['scheme'] = test['scheme']
> + if options.failure_pattern_file:
> + pat_file = os.path.join(os.path.dirname(test['manifest']),
we should add some verification that this pat_file exists, otherwise random changes in the future (many changes end up as copy/paste) to mozharness/taskcluster definitions could end up causing problems.
I would also recomend a message for the log file which would show up on treeherder:
Error: using layout/style/test/stylo-failures.md, if you have a failure, you could have fixed something documented in that file, please reduce the failure count appropriately.
::: testing/mochitest/tests/SimpleTest/SimpleTest.js:236
(Diff revision 1)
> +function usesFailurePatterns() {
> + return Array.isArray(SimpleTest.expected);
> +}
> +
> +function recordIfMatchesFailurePattern(name, diag) {
> + let index = SimpleTest.expected.findIndex(([pat, count]) => {
I am not following this logic, maybe I am rusty in Javascript- could you add a short comment here. I am not sure why we need to match the pattern on name/diag both, and the index bits seem too fluid.
::: testing/mochitest/tests/SimpleTest/SimpleTest.js:254
(Diff revision 1)
> if (parent.TestRunner) {
> + if (!Array.isArray(parent.TestRunner.expected)) {
> - SimpleTest.expected = parent.TestRunner.expected;
> + SimpleTest.expected = parent.TestRunner.expected;
> + } else {
> + // Assertions are checked by the runner.
> + SimpleTest.expected = parent.TestRunner.expected.filter(([pat]) => pat != "ASSERTION");
if there are only assertions here, then what will SimpleTest.expected end up as? will it be []?
::: testing/mochitest/tests/SimpleTest/SimpleTest.js:337
(Diff revision 1)
>
> SimpleTest.todo = function(condition, name, diag) {
> var test = {'result': !!condition, 'name': name, 'diag': diag, todo: true};
> + if (test.result && usesFailurePatterns() &&
> + recordIfMatchesFailurePattern(name, diag)) {
> + // Fliping the result to false so we don't get unexpected result. There
nit: extra 'p' in Flipping
thanks for the good comment here.
Attachment #8837498 -
Flags: review?(jmaher) → review-
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8837498 [details]
Bug 1337674 part 3 - Add failure pattern file support to mochitest.
https://reviewboard.mozilla.org/r/112612/#review114110
> if there are only assertions here, then what will SimpleTest.expected end up as? will it be []?
Yes, it will be [] in that case.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•8 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f740a3966136cde070cf0edd357d3eb91372d988 (The lint error shown in the result should have been fixed in this latest patch.)
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8837498 [details]
Bug 1337674 part 3 - Add failure pattern file support to mochitest.
https://reviewboard.mozilla.org/r/112612/#review114490
thanks for the update
Attachment #8837498 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 17•8 years ago
|
||
I'm going to land the patches as-is in autoland. The tasks wouldn't be green immediately, since there are several dependencies haven't been landed, and there are several failures fixed in the mean time. I'll land followups to adjust the expectation later.
Comment 18•8 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/524075cc07ca
part 1 - Adjust test_load_events_on_stylesheets to not double finish even with error. r=heycam
https://hg.mozilla.org/integration/autoland/rev/a99e8429e40a
part 2 - Disable style system tests written with testharness.js. r=heycam
https://hg.mozilla.org/integration/autoland/rev/92a932f9be3f
part 3 - Add failure pattern file support to mochitest. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/f6cde9d9294a
part 4 - Disable several tests which crash or timeout. r=heycam
Assignee | ||
Comment 19•8 years ago
|
||
Comment 20•8 years ago
|
||
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/794c6254ebe7
followup - Adjust expectation of mochitests of stylo.
Assignee | ||
Comment 21•8 years ago
|
||
Comment 22•8 years ago
|
||
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b80d650eb55a
followup 2 - Skip another crash test for stylo.
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/524075cc07ca
https://hg.mozilla.org/mozilla-central/rev/a99e8429e40a
https://hg.mozilla.org/mozilla-central/rev/92a932f9be3f
https://hg.mozilla.org/mozilla-central/rev/f6cde9d9294a
https://hg.mozilla.org/mozilla-central/rev/794c6254ebe7
https://hg.mozilla.org/mozilla-central/rev/b80d650eb55a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 24•8 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53cc100473f9
followup 3 - Fix assertion check.
Comment 25•8 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•