show list of errors on inconsistent tests #4998
Conversation
Firefox (nightly channel)Testing web-platform-tests at revision 9c19a1f2fd306c251512291c1bcf088c4d198e22 Unstable results
All results1 test ran/url/historical.html
|
Chrome (unstable channel)Testing web-platform-tests at revision 9c19a1f2fd306c251512291c1bcf088c4d198e22 Unstable results
All results1 test ran/url/historical.html
|
|
|
||
| def test_status(self, data): | ||
| self.results[data["test"]][data.get("subtest")][data["status"]] += 1 | ||
| subtest = self.find_or_create_subtest(data) |
There was a problem hiding this comment.
I'm quite tempted to inline this since it should always exist and function calls are rather expensive. It probably doesn't matter though, so up to you.
There was a problem hiding this comment.
In editing this code yesterday, it was easier to understand and make the parallel changes to test and subtest functionality with it broken out, so I'd like to keep it as-is for maintainability.
check_stability.py
Outdated
| for subtest, result in test_results.iteritems(): | ||
| if is_inconsistent(result, iterations): | ||
| inconsistent.append((test, subtest, result)) | ||
| for test_name, test in results.items(): |
There was a problem hiding this comment.
Use iteritems here and below.
Also, since you moved the test status up into the test object rather than making it a "subtest" with no name, this check misses tests that have an unstable overall status. So you either need to revert that change or fix up here (and maybe elsewhere) that assumes the parent test is represented as a subtest with None as the name.
check_stability.py
Outdated
| @@ -592,22 +628,24 @@ def write_results(results, iterations, comment_pr): | |||
| "tests" if len(results) > 1 | |||
| else "test")) | |||
|
|
|||
| for test, test_results in results.iteritems(): | |||
| for test_name, test in results.items(): | |||
|
@jgraham I believe I've addressed your comments. I added a fixup commit for easier review of the change and an unstable test to prove to myself it still worked. I'll squash after you take a look. |
|
Notifying @Sebmaster, @annevk, @domenic, @mikewest, @rubys, @smola, @tomalec, @xiaojunwu, and @zcorpan. (Learn how reviewing works.) |
bobholt
force-pushed
the
check_stability-raise-errors
branch
from
f8ef907
to
560facf
Compare
|
Thanks @jgraham. Just pushed a single commit. For posterity, the current unstable output looks like: |
This is a re-implementation of #4916 with the changes requested in the review after that PR was closed.
@jgraham: I'm happy to continue the review here.