stylo: Land the list of known failures in style system mochitests in-tree and make mochitest-style green

RESOLVED FIXED in Firefox 54

Status

Testing
Mochitest
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

Version 3
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

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: nobody → xidorn+moz
Depends on: 1339301
Depends on: 1339394
Depends on: 1334579
Depends on: 1339341
Green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68ca4593cada8605cd3b310d9d13df4b75db4278
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 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 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 months 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 months 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)
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 months 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+
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 months 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
Created attachment 8838334 [details] [diff] [review]
followup patch to change the expectation

Comment 20

8 months ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/794c6254ebe7
followup - Adjust expectation of mochitests of stylo.
Created attachment 8838358 [details] [diff] [review]
followup 2 - disable another test

Comment 22

8 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b80d650eb55a
followup 2 - Skip another crash test for stylo.
Blocks: 1340434

Comment 23

8 months 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
Last Resolved: 8 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 24

8 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53cc100473f9
followup 3 - Fix assertion check.

Comment 25

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/53cc100473f9
Blocks: 1338992
You need to log in before you can comment on or make changes to this bug.