Bug 162814

<rdar://problem/28574102>

So, it will check elapsed time before continuing to the next test case? Or would we start a JS watchdog timer for each testcase that will abort the entire suite? I fear that if the real problem is the bots are simply too contended or slow, then a watchdog timer in JS will just get stuck like the test and be ineffective. If it's a flake even on a fast machine, then this seems like a decent mitigation. I think a separate issue is that we don't get partial test output when a test times out, even if multiple things have been sent back to the test page. It would be nice to get a warning event sent from the test runner when it's about to kill the test so we could flush any buffered test results.

(In reply to comment #2) > So, it will check elapsed time before continuing to the next test case? Or > would we start a JS watchdog timer for each testcase that will abort the > entire suite? Correct, I was thinking a setTimeout for each test case. Either aborting the entire suite or a single test case. Both are fine, As long as we get some output. The safest thing to do is abort everything, but sometimes that isn't needed and it would be nice to run other test cases that may pass afterwards. > I fear that if the real problem is the bots are simply too contended or > slow, then a watchdog timer in JS will just get stuck like the test and be > ineffective. If it's a flake even on a fast machine, then this seems like a > decent mitigation. Almost always, especially locally when authoring a test, some event doesn't fire and the test times out. The only way I can debug that now is InspectorTest.debug() and a lot of waiting. > I think a separate issue is that we don't get partial test output when a > test times out, even if multiple things have been sent back to the test > page. It would be nice to get a warning event sent from the test runner when > it's about to kill the test so we could flush any buffered test results. Correct. I think this would be easier for TestSuite to do it. Especially considering this is really an issue for our tests, many other tests don't buffer things like us.

Created attachment 366781 [details] Patch Patch for bots to see what tests need rebasing.

Comment on attachment 366781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366781&action=review > Source/WebInspectorUI/ChangeLog:12 > + (AsyncTestSuite.prototype.runTestCases): > + (SyncTestSuite.prototype.runTestCases): I should probably mention that this change makes it so that a single test case failing won't prevent subsequent test cases from running. > Source/WebInspectorUI/UserInterface/Test/TestSuite.js:144 > + let delay = testcase.timeout || 10000; 10000 was derived from the idea that 30000 is the default timeout, and most test suites have more than one or two test cases.

30 seconds is the default timeout unless we are running GuardMalloc or ASan, in which case it's 3x longer (maybe leaks too, I don't remember off-hand). Probably fine to land for a specific investigation, but this will likely make tests more flaky on those queues.

Created attachment 366793 [details] Patch

Created attachment 366794 [details] Patch

Created attachment 366799 [details] Patch

Created attachment 366824 [details] Patch Fix the logic that determines when to add a newline for extra space.

Comment on attachment 366824 [details] 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?

Comment on attachment 366824 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366824&action=review r=me >> 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. OTOH, maybe it will work. If it has to be rolled out, let's be more cautious in followup attempts. > LayoutTests/inspector/canvas/resources/shaderProgram-utilities.js:78 > + whenProgramAdded((program) => { Nice.

Comment on attachment 366824 [details] 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. > > OTOH, maybe it will work. If it has to be rolled out, let's be more cautious in followup attempts. None of the changes in this patch should ever be seen (ideally). In a perfect world, all the tests would pass, so continue on failure wouldn't ever be necessary (the same is true for automatic timeouts). The point of this is to handle the cases where the "bumbling bots" are having issues, or when a test is being written (e.g. "I forgot to add a listener, so it times out"), so that we, as developers, can get more of the output to see what went wrong. >> 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. The nondeterminism is limited to just that test file. My main point here is that if each test case is independent of each other, and one of them times out, the "extra" logging that appears in the output of later test cases could come from the one that timed out, as it's still running (no way to just stop JS execution). >> Source/WebInspectorUI/UserInterface/Test/TestSuite.js:57 >> + return this.runCount - (this.failCount - this.skipCount); > > Why make skips count as passes? This does the opposite of that. It makes skips not count as passes. Skips are treated as failures, so we only want the failures that aren't skips.

Comment on attachment 366824 [details] Patch Clearing flags on attachment: 366824 Committed r243953: <https://trac.webkit.org/changeset/243953>

All reviewed patches have been landed. Closing bug.

The following layout test is failing after changes in https://trac.webkit.org/changeset/243953/webkit on WK1 Release and Debug inspector/canvas/css-canvas-clients.html https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcanvas%2Fcss-canvas-clients.html Diff: https://build.webkit.org/results/Apple%20Mojave%20Release%20WK1%20(Tests)/r244020%20(4238)/inspector/canvas/css-canvas-clients-diff.txt

Found another test that is failing due to the timeout of an existing test that was marked a timeout. inspector/console/console-time.html will fail because of the timeout, then pass on the retry. It appears this test was marked flaky in https://bugs.webkit.org/show_bug.cgi?id=158006 After this revision instead of showing a timeout on the dashboard I believe they've been replaced with the failure. I will update test expectations to skip this test as well. I also updated radars as hopefully we can fix the initial timeout to resolve this failure as well. Dashboard: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fconsole%2Fconsole-time.html Diff: https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r244076%20(3555)/inspector/console/console-time-diff.txt