Closed Bug 1337674 Opened 7 years ago Closed 7 years ago

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

Categories

(Testing :: Mochitest, defect)

Product:
Testing
Component:
Mochitest
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Bug 1337674 part 1 - Adjust test_load_events_on_stylesheets to not double finish even with error.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1337674 part 2 - Disable style system tests written with testharness.js.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1337674 part 3 - Add failure pattern file support to mochitest.
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1337674 part 4 - Disable several tests which crash or timeout.
59 bytes, text/x-review-board-request
heycam
: review+
Details
followup patch to change the expectation
5.02 KB, patch
Details | Diff | Splinter Review
followup 2 - disable another test
1.00 KB, 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: nobody → xidorn+moz
Depends on: 1339301
Depends on: 1339394
Depends on: 1334579
Depends on: 1339341
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 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-
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.
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 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.
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
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/794c6254ebe7
followup - Adjust expectation of mochitests of stylo.
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b80d650eb55a
followup 2 - Skip another crash test for stylo.
Blocks: 1340434
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/53cc100473f9
followup 3 - Fix assertion check.
Blocks: 1338992
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: