This is an archive of the discontinued LLVM Phabricator instance.

sleep and retry on failure to delete temp file in tests
ClosedPublic

Authored by amccarth on Apr 8 2016, 3:13 PM.

Details

Summary

This solves a flaky test on Windows.

We have similar logic elsewhere for log files. I considered trying to combine them, but since they are methods of different objects, there wasn't really a clean approach.

Diff Detail

Repository
rL LLVM

Event Timeline

amccarth updated this revision to Diff 53098.Apr 8 2016, 3:13 PM
amccarth retitled this revision from to sleep and retry on failure to delete temp file in tests.
amccarth updated this object.
amccarth added a reviewer: zturner.
amccarth added a subscriber: lldb-commits.
zturner edited edge metadata.Apr 8 2016, 3:22 PM

How about add a function to lldbsuite\support\fs.py?

def remove_file(file, num_retries, sleep_duration):

Then just call this everywhere you need to. Could even nuke this whole RemoveTempFile method.

amccarth updated this revision to Diff 53100.Apr 8 2016, 3:56 PM
amccarth edited edge metadata.

Consolidated the os.remove and os.unlink calls with a function to do the retries. Tested on Windows.

zturner added inline comments.Apr 8 2016, 4:05 PM
packages/Python/lldbsuite/test/lldbtest.py
1994 ↗(On Diff #53100)

I think the default for num_retries should be 0. Usually you just want to try once without retry.

1995 ↗(On Diff #53100)

You could do for i in range(num_retries+1): and get rid of a bunch of the counter updating and junk

amccarth added inline comments.Apr 8 2016, 4:31 PM
packages/Python/lldbsuite/test/lldbtest.py
1994 ↗(On Diff #53100)

I disagree. Every call site was already doing exactly one retry. I don't see value in making each of them specify that explicitly.

1995 ↗(On Diff #53100)

Fine, but that changes the behavior a bit. My approach avoided that extra sleep after the last retry.

zturner accepted this revision.Apr 8 2016, 4:32 PM
zturner edited edge metadata.

Ahh, you're right, I didn't notice that.

This revision is now accepted and ready to land.Apr 8 2016, 4:32 PM
This revision was automatically updated to reflect the committed changes.