[wptrunner] Add results for expected subtests that didn't run #44125
Conversation
This feature allows Chromium CI to fail builds [0] for changes that forget to clean up expectations for deleted or renamed subtests. Subtest validity cannot be checked statically. Assuming `infrastructure/metadata/**/*.ini` are not stale, this change has no effect on WPT CI or non-Chrome browser testing. Closes web-platform-tests#43499 [0]: https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/1678090/overview
wpt-pr-bot
added
infra
wptrunner
| subtest_names = {subtest.name for subtest in subtest_results} | ||
| for unknown_subtest in sorted(test.subtests - subtest_names): | ||
| subtest_results.append( | ||
| test.subtest_result_cls(unknown_subtest, "NOTRUN", message=None)) |
There was a problem hiding this comment.
If unknown subtests get a "NOTRUN" result, what should rebaseline tool behave?
There was a problem hiding this comment.
Is it possible we do something in the 'WPTResultsProcessor'? If there are unknown subtests, we simply flag that test as unexpectedly failed (if it is not a crash/timeout previously).
We do not need to do anything in Wptrunner, especially we do not need to retry such tests.
There was a problem hiding this comment.
This is usually an unexpected wptrunner NOTRUN, which is mapped to ResultDB unexpected failure that the rebaseline tool downloads.
An expected NOTRUN like:
This is a testharness.js-based test.
[NOTRUN] does not exist
won't cause a failure by itself. Ideally, we wouldn't check subtests anyway during a timeout, so maybe this isn't important to clean up.
Is it possible we do something in the 'WPTResultsProcessor'?
We can try, since WPTResultsProcessor can handle the above expected NOTRUN case. WPTResultsProcessor needs to manipulate the wpt run exit code too then.
There was a problem hiding this comment.
WPTResultsProcessor needs to manipulate the wpt run exit code too then.
Oh yes. I think we could log the return code from Wptrunner, but the actual return value should be decided by run_wpt_tests.py.
|
So do we plan to move the change to Reading your response to my question "If unknown subtests get a "NOTRUN" result, what should rebaseline tool behave?", are you saying for a removed subtest, our plan is to change the expectation for that subtest to "NOTRUN", instead of remove the subtest from the baseline? |
|
Not pursuing this change. Since https://crrev.com/c/5112639 streamlines the result logic significantly, it might be easier to wait for that to land first, which requires solving #44134.
No, the removed subtest should be removed, not added as NOTRUN. I realized that's why this PR doesn't work. |
This feature allows Chromium CI to fail builds for changes that forget to clean up expectations for deleted or renamed subtests. Subtest validity cannot be checked statically.
Assuming
infrastructure/metadata/**/*.iniare not stale, this change has no effect on WPT CI or non-Chrome browser testing.Closes #43499