bpo-39488: skip bigfile test if not enough disk space#18261
bpo-39488: skip bigfile test if not enough disk space#18261vstinner merged 5 commits intopython:masterfrom giampaolo:bigfile-skip
Conversation
Lib/test/test_largefile.py
Outdated
| self.assertTrue(f.seekable()) | ||
|
|
||
|
|
||
| def skip_no_disk_space(): |
There was a problem hiding this comment.
I would prefer to pass explicitly a path, and call it inside the test, not outside (decorator).
Well, like a regular function :-) skip_no_disk_space(path, required).
There was a problem hiding this comment.
I would prefer to pass explicitly a path
Agree. Done.
and call it inside the test, not outside (decorator).
The check is executed right before running the test (and not on module import). Or at least, I guess that is your concern (it was mine too).
Lib/test/test_largefile.py
Outdated
| class TestCopyfile(LargeFileTest, unittest.TestCase): | ||
| open = staticmethod(io.open) | ||
|
|
||
| @skip_no_disk_space(os.getcwd(), size * 3) |
There was a problem hiding this comment.
os.getcwd() can be different when the function is defined and when the test is executed. I suggested to call skip_no_disk_space() inside the function. Something like:
def test_it(self):
skip_no_disk_space(TESTFN, size * 3)
...
TESTFN is usually relative. I'm not sure if skip_no_disk_space() should be made absolute using os.path.abspath().
There was a problem hiding this comment.
Good point. I used os.path.realpath instead.
Lib/test/test_largefile.py
Outdated
| class TestCopyfile(LargeFileTest, unittest.TestCase): | ||
| open = staticmethod(io.open) | ||
|
|
||
| @skip_no_disk_space(TESTFN, size * 3) |
There was a problem hiding this comment.
Why is it *3? I count only two files: TESTFN and TESTFN2. Maybe add a comment explaining why it's *3?
There was a problem hiding this comment.
It's just for extra safety. The required exact disk space would be (file size * 2) (aka 4.7G), but just in case something else writes on disk (especially on builtbots) I opted for (file size * 3) (around 7.2 GB).
There was a problem hiding this comment.
I added a comment:
# Exact required disk space would be (size * 2), but let's give it a
# bit more tolerance.
There was a problem hiding this comment.
If size2 is enough, I suggest to use size2.5 for safety rather than size*3 which is quite huge.
There was a problem hiding this comment.
Makes sense. Done.
Lib/test/test_largefile.py
Outdated
| self.thread.start() | ||
| event.set() | ||
|
|
||
| @skip_no_disk_space(TESTFN, size * 3) |
There was a problem hiding this comment.
Would you mind to copy/paste the command here (and also replace size * 3 with size * 2.5)?
|
Thanks @giampaolo! I tested manually your PR and it works as expected. I merged your PR immediately, since I'm still getting https://bugs.python.org/issue39488 failure frequently on buildbots. Let's see if it's enough to fix these buildbots. |
|
Let's see. =) |
4 participants
https://bugs.python.org/issue39488
https://bugs.python.org/issue39488