bpo-39488: skip bigfile test if not enough disk space by giampaolo · Pull Request #18261 · python/cpython (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation13 Commits5 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

giampaolo

@giampaolo

vstinner

@@ -151,9 +151,23 @@ def test_seekable(self):
self.assertTrue(f.seekable())
def skip_no_disk_space():

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@giampaolo

vstinner

class TestCopyfile(LargeFileTest, unittest.TestCase):
open = staticmethod(io.open)
@skip_no_disk_space(os.getcwd(), size * 3)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I used os.path.realpath instead.

@giampaolo

vstinner

class TestCopyfile(LargeFileTest, unittest.TestCase):
open = staticmethod(io.open)
@skip_no_disk_space(TESTFN, size * 3)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it *3? I count only two files: TESTFN and TESTFN2. Maybe add a comment explaining why it's *3?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment:

    # Exact required disk space would be (size * 2), but let's give it a
    # bit more tolerance.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If size_2 is enough, I suggest to use size_2.5 for safety rather than size*3 which is quite huge.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Done.

@@ -200,6 +213,7 @@ def run(sock):
self.thread.start()
event.set()
@skip_no_disk_space(TESTFN, size * 3)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here :-)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind to copy/paste the command here (and also replace size * 3 with size * 2.5)?

@giampaolo

@giampaolo

@vstinner

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.

@giampaolo