Bug 203652 – Flaky API Test TestWebKitAPI.WebKit.UploadDirectory

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

TestWebKitAPI.WebKit.UploadDirectory seems flaky on macOS. In https://ews-build.webkit.org/#/builders/3/builds/11242, the test failed in run-api-tests step. However, in the immediately next retry step (re-run-api-tests), it passed.

Similar thing happened in: https://ews-build.webkit.org/#/builders/3/builds/11243

[Alexey Proskuryakov]

This test started to crash in r251778, then it turned into a failure with r251795, and then it stopped failing without a clear reason a few commits later.

I suspect that this is a bad test that can leak state when it crashes.

Either way, this is not happening any more.

[Aakash Jain]

Still flaky. Failed just now in https://ews-build.webkit.org/#/builders/3/builds/11251/steps/13/logs/json

[Alexey Proskuryakov]

Build 11251 had 788 API test failures with the patch, and then just this one with the patch removed.

I think that this is a bad test that leaves state sometimes, which makes it fail the next time.

[Radar WebKit Bug Importer]

<rdar://problem/56791786>

[Alex Christensen]

Created attachment 382732 [details]
Patch

[Chris Dumez]

Comment on attachment 382732 [details]
Patch

Can't we remove the directory at the beginning of the test?

[Alex Christensen]

Created attachment 382741 [details]
Patch

[Chris Dumez]

Comment on attachment 382741 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/UploadDirectory.mm:75
> +    [fileManager removeItemAtPath:directory.path error:nil];

Then why not keep the assertion?

[Alex Christensen]

Comment on attachment 382741 [details]
Patch

The point was that after a previous iteration of this test crashes, it leaves behind a file.  removeItemAtPath:error: returns different values depending on whether the file existed before or not, so keeping the assertion would be propagating the flakiness.

[Chris Dumez]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 382741 [details]
> Patch
> 
> The point was that after a previous iteration of this test crashes, it
> leaves behind a file.  removeItemAtPath:error: returns different values
> depending on whether the file existed before or not, so keeping the
> assertion would be propagating the flakiness.

I don't understand why asserting that the folder is not there after removing it would cause flakiness? Do you expect folder removal to fail for some reason?

[Alex Christensen]

Oh, asserting that the folder is not there after removing it would be fine but I think it would be redundant.  I was talking about asserting that removing it succeeded or that the folder is not there before removing, both of which would be flaky.

[Alexey Proskuryakov]

Comment on attachment 382741 [details]
Patch

I would slightly prefer checking for the error or asserting after deleting. Knowing that deletion succeeded will make it easier to reason about future failures, if any occur.

r=me with or without such changes.

[Alex Christensen]

Comment on attachment 382741 [details]
Patch

Committing as-is.  If we see future flakiness we can add such assertions.

[WebKit Commit Bot]

Comment on attachment 382741 [details]
Patch

Clearing flags on attachment: 382741

Committed r251996: <https://trac.webkit.org/changeset/251996>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.