|Summary:||Flaky API Test TestWebKitAPI.WebKit.UploadDirectory|
|Product:||WebKit||Reporter:||Aakash Jain <aakash_jain>|
|Component:||Tools / Tests||Assignee:||Alex Christensen <achristensen>|
|Severity:||Normal||CC:||achristensen, ap, cdumez, commit-queue, webkit-bot-watchers-bugzilla, webkit-bug-importer|
Description Aakash Jain 2019-10-30 18:23:30 PDT
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
Comment 1 Alexey Proskuryakov 2019-10-30 23:37:58 PDT
Comment 2 Aakash Jain 2019-10-31 05:41:23 PDT
Still flaky. Failed just now in https://ews-build.webkit.org/#/builders/3/builds/11251/steps/13/logs/json
Comment 3 Alexey Proskuryakov 2019-10-31 12:54:49 PDT
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.
Comment 6 Chris Dumez 2019-11-04 08:01:50 PST
Comment on attachment 382732 [details] Patch Can't we remove the directory at the beginning of the test?
Comment 8 Chris Dumez 2019-11-04 08:18:17 PST
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 9 Alex Christensen 2019-11-04 08:35:54 PST
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.
Comment 10 Chris Dumez 2019-11-04 08:44:01 PST
(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?
Comment 11 Alex Christensen 2019-11-04 08:49:53 PST
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 12 Alexey Proskuryakov 2019-11-04 09:06:19 PST
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 13 Alex Christensen 2019-11-04 09:07:26 PST
Comment on attachment 382741 [details] Patch Committing as-is. If we see future flakiness we can add such assertions.
Comment 14 WebKit Commit Bot 2019-11-04 09:50:42 PST
Comment on attachment 382741 [details] Patch Clearing flags on attachment: 382741 Committed r251996: <https://trac.webkit.org/changeset/251996>
Comment 15 WebKit Commit Bot 2019-11-04 09:50:44 PST
All reviewed patches have been landed. Closing bug.