Skip to content

add feature to view fixture source location in invocations with --fixtures-per-test option #8626

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

Merged
merged 8 commits into from
May 14, 2021

Conversation

kumiDa
Copy link
Contributor

@kumiDa kumiDa commented May 3, 2021

This implements the proposal in issue #8606

The following are the changes made to the behaviour:

  • Always showing the path to the fixtures when using the --fixtures-per-test option
  • Only showing the first line of the docstrings unless --verbose is given

@kumiDa
Copy link
Contributor Author

kumiDa commented May 3, 2021

I have not made an entry into changelog and documentation.
I have put them on hold to get your reviews on these change and add them.

@The-Compiler, @nicoddemus Please review the PR and let me know if there are changes to be made.

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.

Yay, thank you! 🎉 Looks good overall, I commented on two small things.

@kumiDa
Copy link
Contributor Author

kumiDa commented May 4, 2021

@The-Compiler, Thanks for the review.
I made the suggested changes.

Can you please take a look and let me know if documentation and changelog for this change can be added?

@kumiDa kumiDa requested a review from The-Compiler May 4, 2021 09:17
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.

Thanks! One thing I noticed just now is that it kind of breaks with docstrings where the summary spans across multiple lines, like e.g. with mocked_urlopen:

@pytest.fixture
def mocked_urlopen(self, monkeypatch: MonkeyPatch):
"""Monkeypatch the actual urlopen calls done by the internal plugin
function that connects to bpaste service."""
calls = []

This now displays as:

-------------------- fixtures used by test_create_new_paste --------------------
------------------------ (testing/test_pastebin.py:163) ------------------------
mocked_urlopen -- testing/test_pastebin.py:129
    Monkeypatch the actual urlopen calls done by the internal plugin
monkeypatch -- src/_pytest/monkeypatch.py:29
    A convenient fixture for monkey-patching.
pastebin -- testing/test_pastebin.py:88
    no docstring available

so ideally, we'd instead split on the first empty line. Perhaps something like .split('\n\n') would already be good enough?

Also, it looks like this PR only adjusts --fixtures-per-test - I think it'd be good to adjust --fixtures in the same way as well.

@kumiDa
Copy link
Contributor Author

kumiDa commented May 4, 2021

@The-Compiler,
Use of the .split('\n\n') seems to be a good idea to tackle cases where the docstring sentence is spread across different lines. Introducing this would mean that we would be returning the first paragraph of the docstring if it is paragraphed else the entire content.

Hope this behaviour is acceptable.

@kumiDa
Copy link
Contributor Author

kumiDa commented May 4, 2021

@The-Compiler,

so ideally, we'd instead split on the first empty line. Perhaps something like .split('\n\n') would already be good enough?

This is addressed in this commit: c0dc982

I think it'd be good to adjust --fixtures in the same way as well.

This is addressed in this commit: 02f026f

@kumiDa kumiDa requested a review from The-Compiler May 4, 2021 15:31
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.

Thanks for the update! Also a great idea to add some test for the docstring parsing.

One thing I didn't realize before looking at the updated diff is that --fixtures --verbose actually did show the path in a different color:

image

I think this helps readability a bit, and IMHO it'd be good to keep that. What do you think?

Other than that, this will still need a changelog entry, but seems fine otherwise. I don't think any other changes to the documentation are needed, but maybe I missed something.

@kumiDa kumiDa requested a review from The-Compiler May 6, 2021 09:08
@kumiDa
Copy link
Contributor Author

kumiDa commented May 7, 2021

@The-Compiler, I have made the suggested changes.
Can you please review the same and let me know if there's anything else that has to be improved.

@The-Compiler
Copy link
Member

I've seen your review request and will take another look, but it'll probably be somewhen next week until I get around to it.

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.

Thanks, the output looks great now! I commented on two changes where I'm unsure what your intentions were - after that I think this should be ready to merge!

@kumiDa kumiDa requested a review from The-Compiler May 11, 2021 10:06
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.

Thanks again! Makes this output much more useful, in my opinion!

funcargspec = argname
tw.line(funcargspec, green=True)
bestrel = get_best_relpath(fixture_def.func)
tw.write(f"{argname}", green=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@The-Compiler, I noticed your suggestion to use argname in this line as a direct variable reference instead of using it as a formatted string. I am not able to find that comment to reference it.

I think that would be a good thing to do and keeps things consistent. Please let me know if this small change can go in as a separate PR.

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.

2 participants