Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test.regrtest should complain if a test doesn't remove temporary files #66584

Closed
vstinner opened this issue Sep 11, 2014 · 23 comments
Closed

test.regrtest should complain if a test doesn't remove temporary files #66584

vstinner opened this issue Sep 11, 2014 · 23 comments
Assignees
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@vstinner
Copy link
Member

BPO 22390
Nosy @pitrou, @vstinner, @ezio-melotti, @voidspace, @serhiy-storchaka
Files
  • regrtest_warn_lost_files.patch
  • fix_tests.patch
  • regrtest_warn_lost_files2.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2015-03-30.07:10:41.991>
    created_at = <Date 2014-09-11.14:30:52.008>
    labels = ['type-feature', 'tests']
    title = "test.regrtest should complain if a test doesn't remove temporary files"
    updated_at = <Date 2015-03-30.08:41:08.553>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2015-03-30.08:41:08.553>
    actor = 'vstinner'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2015-03-30.07:10:41.991>
    closer = 'serhiy.storchaka'
    components = ['Tests']
    creation = <Date 2014-09-11.14:30:52.008>
    creator = 'vstinner'
    dependencies = []
    files = ['36601', '36647', '36894']
    hgrepos = []
    issue_num = 22390
    keywords = ['patch']
    message_count = 23.0
    messages = ['226777', '226786', '226787', '226788', '226792', '226793', '227015', '228584', '229196', '229200', '229234', '236080', '239488', '239525', '239527', '239529', '239532', '239533', '239534', '239536', '239539', '239575', '239582']
    nosy_count = 6.0
    nosy_names = ['pitrou', 'vstinner', 'ezio.melotti', 'michael.foord', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue22390'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @vstinner
    Copy link
    Member Author

    A change in test_glob of issue bpo-13968 started to fail because a previous test created temporary files but didn't remove them.

    test.regrtest should at least emit a warning if the temporary directory used to run tests is not empty before removing it.

    @vstinner vstinner added the tests Tests in the Lib/test dir label Sep 11, 2014
    @serhiy-storchaka
    Copy link
    Member

    Here is a patch. It warns if new files or directories are left after test run in current directory and removes those of them which starts with TESTFN (i.e. TESTFN_UNICODE, TESTFN + "2", TESTFN + ".py" and other names used in tests).

    @serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Sep 11, 2014
    @vstinner
    Copy link
    Member Author

    I would prefer to fix tests instead of trying to remove arbitrary files. I'm not sure that "fn.startswith(support.TESTFN)" check is safe enough.

    Maybe we should not remove TESTFN neither. You are supposed to be able to run tests without regrtest.

    @vstinner
    Copy link
    Member Author

    I ran the test suite with the patch applied:

    7 tests altered the execution environment:
    test_imp test_import test_pdb test_posix test_source_encoding
    test_support test_threaded_import

    I will try to investigate these warnings.

    @serhiy-storchaka
    Copy link
    Member

    I would prefer to fix tests instead of trying to remove arbitrary files. I'm
    not sure that "fn.startswith(support.TESTFN)" check is safe enough.

    This allow other tests which leaks the same file to be reported too.

    support.TESTFN contains process ID so it is unique enough.

    @serhiy-storchaka
    Copy link
    Member

    I will try to investigate these warnings.

    Run tests with the -vv option.

    @vstinner
    Copy link
    Member Author

    fix_tests.patch: Fix the 7 tests which create files without removing them. 4 tests (test_imp, test_pdb, test_source_encoding and test_support) create a __pycache__ directory. Maybe this directory should be ignored by regrtest?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 5, 2014

    New changeset 8cf8bff3569e by Victor Stinner in branch '3.4':
    Issue bpo-22390: Remove files created by tests
    https://hg.python.org/cpython/rev/8cf8bff3569e

    @serhiy-storchaka
    Copy link
    Member

    How about regrtest_warn_lost_files.patch?

    @vstinner
    Copy link
    Member Author

    This allow other tests which leaks the same file to be reported too.

    I don't understand your answer. The "fn.startswith(support.TESTFN)" test is not used in get_files().

    If I understood correctly, your patch changes two things:

    • it now reports files created for tests but not deleted (purpose of this issue)
    • it removes more files than before

    I don't understand why you want to remove more files than before. You may open a different issue, or at least explain the rationale.

    I never see any forgotten test file after running tests, so I don't see why you are worried because of them. And with your first patch, we will now noticed forgotten files, so we can just fix tests.

    @serhiy-storchaka
    Copy link
    Member

    I don't understand why you want to remove more files than before. You may
    open a different issue, or at least explain the rationale.

    I thought it would be good idea slightly extend this cleanup while we are
    here. I'm not motivated enough to open a different issue.

    Well, here is a patch which removes only TESTFN. It is still improved, uses
    support.unlink and support.rmtree instead of os.unlink and shutil.rmtree.

    You can just drop cleanup code at all if you prefer. All is good to me.

    I never see any forgotten test file after running tests, so I don't see why
    you are worried because of them.

    This is because regrtest creates temporary directory and goes to it. But when
    you execute Python test directly, test files are created in the current
    directory.

    And with your first patch, we will now
    noticed forgotten files, so we can just fix tests.

    But we will noticed only one about test at the time if several tests forgot
    the same file. This will needed several iterations.

    @serhiy-storchaka
    Copy link
    Member

    What about this issue?

    @serhiy-storchaka
    Copy link
    Member

    Victor?

    @vstinner
    Copy link
    Member Author

    regrtest_warn_lost_files2.patch looks good to me.

    I just ran the test suite with "./python -m test -j0 -rW" on Linux, I didn't get any warning.

    When I modified test_os to create a file "x", it was noticed by your patch.

    Go ahead.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 29, 2015

    New changeset f40984e7ceea by Serhiy Storchaka in branch '2.7':
    Issue bpo-22390: test.regrtest now emits a warning if temporary files or
    https://hg.python.org/cpython/rev/f40984e7ceea

    New changeset 05e6bab4db8f by Serhiy Storchaka in branch '3.4':
    Issue bpo-22390: test.regrtest now emits a warning if temporary files or
    https://hg.python.org/cpython/rev/05e6bab4db8f

    New changeset bed806c9eb4c by Serhiy Storchaka in branch 'default':
    Issue bpo-22390: test.regrtest now emits a warning if temporary files or
    https://hg.python.org/cpython/rev/bed806c9eb4c

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 29, 2015

    New changeset 207db4338706 by Victor Stinner in branch '2.7':
    Issue bpo-22390: Fix typo in regrtest, support => test_support
    https://hg.python.org/cpython/rev/207db4338706

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 29, 2015

    New changeset fb5c3528d0d7 by Victor Stinner in branch '2.7':
    Issue bpo-22390: Fix test_aifc to remove the created file
    https://hg.python.org/cpython/rev/fb5c3528d0d7

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 29, 2015

    New changeset c5c31adbefeb by Victor Stinner in branch '2.7':
    Issue bpo-22390: Fix test_pdb to remove created bar.pyc file
    https://hg.python.org/cpython/rev/c5c31adbefeb

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 29, 2015

    New changeset 33e48141d83f by Victor Stinner in branch '2.7':
    Issue bpo-22390: Fix test_gzip, remove temporary file
    https://hg.python.org/cpython/rev/33e48141d83f

    @vstinner
    Copy link
    Member Author

    On Windows, test_idle modifies os.environ: TCL_xxx and TIX_xxx (sorry
    for "xxx", I don't remember the full variable name) are added. And
    test_platform modifies os.environ['PATH'].

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 30, 2015

    New changeset 6ef2cacec2e9 by Victor Stinner in branch '2.7':
    Issue bpo-22390: Fix test_gzip if unicode filename doesn't work
    https://hg.python.org/cpython/rev/6ef2cacec2e9

    @serhiy-storchaka
    Copy link
    Member

    Thank you for fixing backported patch and tests Victor.

    @vstinner
    Copy link
    Member Author

    Serhiy Storchaka added the comment:

    Thank you for fixing backported patch and tests Victor.

    No problem, thanks for your enhancement of regrtest ;-) I proposed the
    idea and you implemented it, great team work!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants