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

Fix cwd explosion #1593

Merged
merged 1 commit into from
Jun 10, 2016
Merged

Conversation

marscher
Copy link
Contributor

@marscher marscher commented Jun 7, 2016

This contains only a small fix for the bug mentioned in #1235, so it should go to master.

@@ -925,3 +925,11 @@ def test_repr_traceback_with_unicode(style, encoding):
repr_traceback = formatter.repr_traceback(e_info)
assert repr_traceback is not None

@pytest.mark.xfail
def test_cwd_deleted_(tmpdir):
Copy link
Member

Choose a reason for hiding this comment

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

Could you use testdir instead? This way we can be sure the sub-test is actually failing, while the actual test_cwd_deleted then passes.

@nicoddemus
Copy link
Member

Thanks for taking the time to work on this! 😁

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 92.207% when pulling c3a9700 on marscher:fix_cwd_explosion into 70fdab4 on pytest-dev:master.

@@ -925,3 +925,14 @@ def test_repr_traceback_with_unicode(style, encoding):
repr_traceback = formatter.repr_traceback(e_info)
assert repr_traceback is not None


@pytest.mark.xfail
def test_cwd_deleted_(testdir):
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for not being clearer, I meant something like this:

def test_cwd_deleted(testdir):
    testdir.makepyfile("""
        def test(tmpdir):
            tmpdir.chdir()
            tmpdir.remove()
            assert False         
    """)
    result = testdir.runpytest() 
    result.stdout.fnmatch_lines(['* 1 failed in *'])
    assert 'INTERNALERROR' not in result.stdout.str() + result.stderr.str()

This way we make sure that when the test passes we are observing the correct behavior.

@marscher
Copy link
Contributor Author

marscher commented Jun 8, 2016

Ah okay. Thank you!

@@ -10,6 +10,9 @@

*

* Fix exception visualization in case the current working directory (CWD) gets
deleted during testing. Fixes (`#1235`). Thanks `@bukzor` for reporting.
Copy link
Member

Choose a reason for hiding this comment

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

While you are at it, please add a and@marscher_ for the PR. 😁

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 92.207% when pulling 7c3bbf8 on marscher:fix_cwd_explosion into 70fdab4 on pytest-dev:master.

@marscher
Copy link
Contributor Author

marscher commented Jun 8, 2016

Now paths are still relative if cwd exists and absolute if it doesn't. Is this really such a huge game changer to target feature?

@The-Compiler
Copy link
Member

I think that'd be fine. If you have a pytest crash, $tool isn't going to be able to parse the output either, so the worst that can happen is that it still can't.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 92.212% when pulling 09d163a on marscher:fix_cwd_explosion into 70fdab4 on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 92.207% when pulling cd30f3d on marscher:fix_cwd_explosion into 70fdab4 on pytest-dev:master.

@nicoddemus
Copy link
Member

I agree, it can't possibly break anything, so in this case I think it is OK to go to master. @RonnyPfannschmidt do you agree?

Thanks again @marscher for the work and patience! 👍

@RonnyPfannschmidt
Copy link
Member

I propose merging to features and doing 2.10 next weekend

@nicoddemus
Copy link
Member

No problem in releasing 2.10 soon (although next weekend I'll be in Freiburg already), but do you have anything against merging this to master? I would rather not ask @marscher to rebase on features unless really required.

@marscher
Copy link
Contributor Author

marscher commented Jun 9, 2016 via email

@RonnyPfannschmidt
Copy link
Member

Agreed

@RonnyPfannschmidt RonnyPfannschmidt merged commit 577cce2 into pytest-dev:master Jun 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants