None of our webkitpy tests should ever be flakey. Running the test stand-alone pretty readily reproduces it.
<rdar://problem/34769470>
Created attachment 322390 [details] Patch
*** Bug 175692 has been marked as a duplicate of this bug. ***
*** Bug 169777 has been marked as a duplicate of this bug. ***
Sweet!
Aakash asked for an example of what this failure looks like: AssertionError: "MOCK run_command: ['perl', 'Tools/Scripts/webkit-build-directory', '--mac'], cw [truncated]... != "MOCK run_and_throw_if_fail: ['Tools/Scripts/run-api-tests', '--release', '--jso [truncated]... - MOCK run_command: ['perl', 'Tools/Scripts/webkit-build-directory', '--mac'], cwd=/mock-checkout MOCK run_and_throw_if_fail: ['Tools/Scripts/run-api-tests', '--release', '--json-output=/tmp/api_test_results.json'], cwd=/mock-checkout It is the first command, ['perl', 'Tools/Scripts/webkit-build-directory', '--mac'], which is the problem.
Created attachment 322578 [details] Patch
Aakash wanted to isolate this logic to the tests which have these problems.
Comment on attachment 322578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322578&action=review > Tools/Scripts/webkitpy/tool/steps/steps_unittest.py:40 > +def output_capture_logs_end_with(testcase, function, args=[], kwargs={}, expected_logs=None): This method name can be improved. Should have assert in the name. Also can we add a comment here describing why we need this? > Tools/Scripts/webkitpy/tool/steps/steps_unittest.py:46 > + (_, _, logs_string) = capture.restore_output() It might be a good idea to also save stdout_string, stderr_string and assert them to be empty.
Created attachment 322585 [details] Patch for landing
Comment on attachment 322585 [details] Patch for landing We use OutputCapture().assert_outputs() all over the code base. I do not understand why this solution is specific to Tools/Scripts/webkitpy/tool/steps/steps_unittest.py.
Comment on attachment 322585 [details] Patch for landing After talking with Jonathan today in person, I suggest that we have some kind of setup function that is invoked before running any test function in steps_unittest.py that calls _build_path() to prime it. This will ensure that OutputCapture().assert_outputs() gives consistent results regardless of executive order of the individual unit test functions.
(In reply to Daniel Bates from comment #12) > executive order of the individual unit test functions. *execution
We should also try to audit for this bug throughout the other unit tests.
(In reply to Daniel Bates from comment #14) > We should also try to audit for this bug throughout the other unit tests. I'll work on auditing these problems...this particular flavor of test flakiness should be pretty easy to find.
Created attachment 322603 [details] Patch
Comment on attachment 322603 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322603&action=review > Tools/ChangeLog:12 > + There are a number of unit tests in webkitpy which are flakey because they > + neglected to consider logging inside memoized functions. Since we're capturing > + logging in a block, if these memoized functions have been run, we don't see the > + logging. But if they haven't yet been run, we will see the logging. This is complicated and abstract. We should just explain the particular problem that is occurring in steps_unittest.py. Use my comment below as a template for how to explain this issue. > Tools/Scripts/webkitpy/tool/steps/steps_unittest.py:44 > + # Port._build_path() caches the result of a function which will use the executive. This can > + # cause flakiness because we don't know the state of the cache, so force a value to be cached. Maybe something like this would be better: Port._build_path() calls Tools/Scripts/webkit-build-directory and caches the result. When capturing output this can cause the first invocation of this function to have additional output compared to subsequent invocations. This can cause flakiness when test order changes. We explicitly call _build_path() before running a unit test to avoid such flakiness between test runs. > Tools/Scripts/webkitpy/tool/steps/steps_unittest.py:45 > + tool = MockTool(log_executive=True) Is it necessary to pass True for log_executive? Can we pass False? > Tools/Scripts/webkitpy/tool/steps/steps_unittest.py:46 > + tool._deprecated_port = DeprecatedPort() This is not necessary. Please remove.
Created attachment 322613 [details] Patch for landing
Comment on attachment 322613 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=322613&action=review > Tools/Scripts/webkitpy/tool/steps/steps_unittest.py:48 > + Are we sure this will affect all the other mock tools instances?
(In reply to Daniel Bates from comment #19) > Comment on attachment 322613 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322613&action=review > > > Tools/Scripts/webkitpy/tool/steps/steps_unittest.py:48 > > + > > Are we sure this will affect all the other mock tools instances? If so, feel free to cq+.
(In reply to Daniel Bates from comment #19) > Comment on attachment 322613 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322613&action=review > > > Tools/Scripts/webkitpy/tool/steps/steps_unittest.py:48 > > + > > Are we sure this will affect all the other mock tools instances? It won't affect all the other mock tools instances, but it does effect all of the tests running steps.RunTests(...). It is actually steps.RunTests(...) which runs Port._build_path(). As an additional note, along with looking through webkitpy to find invocations of steps.RunTests(...), I also ran the entire test suite one test at a time, to see if any other unit tests were suffering from this same kind of flakiness. I was able to confirm that every test in test-webkitpy can be run independently, meaning after this change, no test is dependent on a previously run test. Note that this technique does not definitively confirm that every test is independent of previous test runs (ie, test-a sets some state which then causes test-b to fail)
Comment on attachment 322613 [details] Patch for landing Clearing flags on attachment: 322613 Committed r222856: <http://trac.webkit.org/changeset/222856>
All reviewed patches have been landed. Closing bug.