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

Revert change to traceback repr #7535

Merged
merged 3 commits into from Jul 24, 2020

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Jul 23, 2020

This PR ammends #7274, undoing an incidental change to the output string and therefore fixes #7534.

cc @nicoddemus @hroncok

@Zac-HD Zac-HD added this to the 6.0 milestone Jul 23, 2020
@The-Compiler
Copy link
Member

Would be good to have a test to prevent this kind of regression in the future? It looks like testing/code/test_excinfo.py would be the best place.

@Zac-HD
Copy link
Member Author

Zac-HD commented Jul 23, 2020

I did wonder about that, but couldn't think of a test that would catch similar issues without being rather fragile to non-problems like the names of parent directories. Personally I'm OK with relying on low code churn here, but if anyone wants to push a test to this branch be my guest 🙂

@nicoddemus
Copy link
Member

I did wonder about that, but couldn't think of a test that would catch similar issues without being rather fragile to non-problems like the names of parent directories. Personally I'm OK with relying on low code churn here, but if anyone wants to push a test to this branch be my guest

I think a test is important here, we would just need to use a regex to make the test independent from the actual file path.

@@ -262,7 +262,12 @@ def __str__(self) -> str:
raise
except BaseException:
line = "???"
return " File %r:%d in %s\n %s\n" % (self.path, self.lineno + 1, name, line)
return " File %r:%d in %s\n %s\n" % (
Copy link
Member

Choose a reason for hiding this comment

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

I think we should show the non-repr of the path here:

Suggested change
return " File %r:%d in %s\n %s\n" % (
return " File %s:%d in %s\n %s\n" % (

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so - Python also quotes the path in its tracebacks:

Traceback (most recent call last):
  File "test.py", line 1, in <module>
    raise Exception

Copy link
Member

Choose a reason for hiding this comment

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

Ah, though Python uses " in any case:

  File "test"foo.py", line 1, in <module>

so I suppose we could do File \"%s\" here instead (and drop the str(...) again?).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so - Python also quotes the path in its tracebacks:

Ahh you are right. 👍

so I suppose we could do File "%s" here instead (and drop the str(...) again?).

Right on. 😁

Copy link
Member

Choose a reason for hiding this comment

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

@The-Compiler

File "test.py", line 1, in

The code seems to (before my oversight) do

File "test.py":1 in

Which looks kinda weird. Should we indeed change it to match the Python format, i.e File "%s", line %d, in %s?

Though that would break werkzeug as well, so not for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I hadn't noticed that. I guess let's just stay with the previous format for now then?

Copy link
Member

Choose a reason for hiding this comment

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

Actually I noticed previously this used ' instead of ", based on the failure reported in #7534:

        traceback = "".join(str(line) for line in baz_exc.traceback)
>       assert "bb.py':1" in traceback  # a bit different than typical python tb

(Nevermind this, replied to a stale comment hehehe)

@Zac-HD
Copy link
Member Author

Zac-HD commented Jul 24, 2020

@nicoddemus - I've pushed a final small tweak, because "'%s'" % (any_string,) is not quite the same as "%r" % (any_string,) - e.g. when any_string contains a ' single-quote.

This last patch therefore restores the previous behaviour exactly, and adds a comment explaining why we're not matching Python. IMO if we're going to make a change at all, we should do it to exactly match Cpython and be done with it; but that's a topic for later.

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Looks great! Not sure if it needs a changelog entry or not, though?

@nicoddemus
Copy link
Member

Looks great! Not sure if it needs a changelog entry or not, though?

I think it makes sense because the change was released in 6.0.0rc1, and I gather we will keep the release candidate changelogs in the final version.

@nicoddemus
Copy link
Member

This last patch therefore restores the previous behaviour exactly, and adds a comment explaining why we're not matching Python.

Awesome. 👍

@The-Compiler
Copy link
Member

One step closer to a final 6.0 then! 🎉 Thanks everyone!

@The-Compiler The-Compiler merged commit 3a060b7 into pytest-dev:master Jul 24, 2020
@Zac-HD Zac-HD deleted the revert-raises-traceback-change branch July 24, 2020 11:54
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.

pytest 6: Traceback in pytest.raises contains repr of py.path.local
4 participants