[webkit-reviews] review denied: [Bug 162814] Web Inspector: TestSuite test cases should have their own timeout to ensure tests fail with output instead of timeout by test runner : [Attachment 366824] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 162814: Web Inspector: TestSuite test cases should have their own timeout
to ensure tests fail with output instead of timeout by test runner
https://bugs.webkit.org/show_bug.cgi?id=162814

Attachment 366824: Patch

https://bugs.webkit.org/attachment.cgi?id=366824&action=review




--- Comment #21 from Brian Burg <bburg at apple.com> ---
Comment on attachment 366824
  --> https://bugs.webkit.org/attachment.cgi?id=366824
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366824&action=review

> Source/WebInspectorUI/ChangeLog:15
> +	   This change also relaxes the expectation that any individual test
case failure will stop the

This patch is trying to do too many things. Given that it's making a risky
infrastructure change that's likely to trip up the bumbling bots, I think you
should split out the "continue on failure" part out. That part seems to be
responsible for most of the test and result diffs, yet it seems less
contentious and less likely to be rolled out and back in repeatedly than the
"add a timeout" part of the patch.

> Source/WebInspectorUI/ChangeLog:21
> +	   "out of nowhere" in the middle of other tests.

It sounds like the only way we can make test cases have non-flaky results is by
disabling continue-on-fail. For if a failure is a timeout, any further tests
run in the same context could be tainted by residual code/logging still running
from the timeout'd test.

On second thought, if all the nondeterminism is limited to just the one test
file, then that's probably okay because it will be marked as failing anyway.

> Source/WebInspectorUI/UserInterface/Test/TestSuite.js:57
> +	   return this.runCount - (this.failCount - this.skipCount);

Why make skips count as passes?