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

RESOLVED FIXED in Firefox 54

Status

RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
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

2 years ago
Assignee: nobody → xidorn+moz
(Assignee)

Updated

2 years ago
Depends on: 1339301
(Assignee)

Updated

2 years ago
Depends on: 1339394
(Assignee)

Updated

2 years ago
Depends on: 1334579
(Assignee)

Updated

2 years ago
Depends on: 1339341
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 years ago
Created attachment 8838334 [details] [diff] [review]
followup patch to change the expectation

Comment 20

2 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

2 years ago
Created attachment 8838358 [details] [diff] [review]
followup 2 - disable another test

Comment 22

2 years ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b80d650eb55a
followup 2 - Skip another crash test for stylo.
(Assignee)

Updated

2 years ago
Blocks: 1340434

Comment 24

2 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53cc100473f9
followup 3 - Fix assertion check.
(Assignee)

Updated

2 years ago
Blocks: 1338992

Updated

10 months ago
Blocks: 1430946
You need to log in before you can comment on or make changes to this bug.