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

Use abspath for errors when cwd changes during testing #7220

Merged
merged 3 commits into from May 30, 2020

Conversation

nicoddemus
Copy link
Member

Fixes #6428

Supersedes #6429

@nicoddemus nicoddemus added the needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch label May 16, 2020
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

otherwise, patch looks good

testing/test_nodes.py Show resolved Hide resolved
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

nice one

@bluetech
Copy link
Member

Just to make sure this is the best solution, some alternatives might be:

  1. Drop the abspath business, use invocation_dir always -- i.e., disregard the cwd.
  2. Keep the current behavior -- they do change the cwd after all.
  3. Reset the cwd ourselves in opportune times.

2 & 3 are probably not good ideas, but 1 does make sense to me, I'm not sure we should really try to adapt to changing cwd after startup...

@nicoddemus
Copy link
Member Author

Drop the abspath business, use invocation_dir always -- i.e., disregard the cwd.

That's a good idea, but it would probably require deeper changes in excinfo.getrepr (which only supports abspath currently).

but 1 does make sense to me, I'm not sure we should really try to adapt to changing cwd after startup...

Not sure, the use case presented seems reasonable to me, a fixture is changing cwd during setup and restoring it during teardown. And the problem also manifests itself if we use monkeypatch.chdir.

@bluetech
Copy link
Member

OK, if changing to be relative to invocation_dir is too hard, LGTM. Would maybe add a comment like:

# excinfo.getrepr() formats paths relative to the CWD if `abspath` is False.
# It is possible for a fixture/test to change the CWD while this code runs, which
# would then result in the user seeing confusing paths in the failure message.
# To fix this, if the CWD changed, always display the full absolute path.
# It will be better to just always display paths relative to invocation_dir, but
# this requires a lot of plumbing.

BTW, presumably this is a problem also for other places that call ExceptionInfo.getrepr (if there are any)?

blueyed and others added 3 commits May 30, 2020 20:02
On Windows specifically is common to have drives diverging just by
casing ("C:" vs "c:"), depending on the cwd provided by the user.
@nicoddemus nicoddemus merged commit a146559 into pytest-dev:master May 30, 2020
@nicoddemus nicoddemus deleted the issue-6428 branch May 30, 2020 23:14
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request May 30, 2020
@nicoddemus
Copy link
Member Author

Backport: #7285

nicoddemus added a commit that referenced this pull request May 31, 2020
[5.4] Merge pull request #7220 from nicoddemus/issue-6428
@nicoddemus nicoddemus added backported PR has been backported to the current bug-fix branch and removed needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch labels May 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported PR has been backported to the current bug-fix branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong path to test file when directory changed in fixture
5 participants