| Summary: | Flaky API Test TestWebKitAPI.WebKit.UploadDirectory | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Aakash Jain <aakash_jain> | ||||||
| Component: | Tools / Tests | Assignee: | Alex Christensen <achristensen> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | achristensen, ap, cdumez, commit-queue, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | Other | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Aakash Jain
2019-10-30 18:23:30 PDT
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. Still flaky. Failed just now in https://ews-build.webkit.org/#/builders/3/builds/11251/steps/13/logs/json 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. Created attachment 382732 [details]
Patch
Comment on attachment 382732 [details]
Patch
Can't we remove the directory at the beginning of the test?
Created attachment 382741 [details]
Patch
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? 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.
(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? 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. 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.
Comment on attachment 382741 [details]
Patch
Committing as-is. If we see future flakiness we can add such assertions.
Comment on attachment 382741 [details] Patch Clearing flags on attachment: 382741 Committed r251996: <https://trac.webkit.org/changeset/251996> All reviewed patches have been landed. Closing bug. |