Bug 177751 – webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_api is flakey

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

None of our webkitpy tests should ever be flakey.

Running the test stand-alone pretty readily reproduces it.

[Radar WebKit Bug Importer]

<rdar://problem/34769470>

[Jonathan Bedard]

Created attachment 322390 [details]
Patch

[Ryan Haddad]

*** Bug 175692 has been marked as a duplicate of this bug. ***

[Ryan Haddad]

*** Bug 169777 has been marked as a duplicate of this bug. ***

[Srinivasan Vijayaraghavan]

Sweet!

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 322578 [details]
Patch

[Jonathan Bedard]

Aakash wanted to isolate this logic to the tests which have these problems.

[Aakash Jain]

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.

[Jonathan Bedard]

Created attachment 322585 [details]
Patch for landing

[Daniel Bates]

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.

[Daniel Bates]

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.

[Daniel Bates]

(In reply to Daniel Bates from comment #12)
> executive order of the individual unit test functions.

*execution

[Daniel Bates]

We should also try to audit for this bug throughout the other unit tests.

[Jonathan Bedard]

(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.

[Jonathan Bedard]

Created attachment 322603 [details]
Patch

[Daniel Bates]

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.

[Jonathan Bedard]

Created attachment 322613 [details]
Patch for landing

[Daniel Bates]

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?

[Daniel Bates]

(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+.

[Jonathan Bedard]

(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)

[WebKit Commit Bot]

Comment on attachment 322613 [details]
Patch for landing

Clearing flags on attachment: 322613

Committed r222856: <http://trac.webkit.org/changeset/222856>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.