Skip to content

Modify documentation to use .stash when storing test results. #10535

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 6 commits into from
Nov 27, 2022

Conversation

ykadowak
Copy link
Contributor

To store test results, it's better to use .stash instead of adding attributes on items. However, the documentation currently advises to add attributes. So it's probably better to update the documentation.

Some contexts:
#10442 (comment)
#10517

@ykadowak
Copy link
Contributor Author

@The-Compiler @nicoddemus
What do you think?

Comment on lines 909 to 912
if your_unique_key not in item.stash:
item.stash[your_unique_key] = {rep.when: rep.passed}
else:
item.stash[your_unique_key][rep.when] = rep.passed
Copy link
Member

Choose a reason for hiding this comment

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

Can use setdefault to make this more succinct:

Suggested change
if your_unique_key not in item.stash:
item.stash[your_unique_key] = {rep.when: rep.passed}
else:
item.stash[your_unique_key][rep.when] = rep.passed
item.stash.setdefault(your_unique_key, {})[rep.when] = rep.passed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know this. Thanks.
ca2cc3d

Also refactored the tmpdir.py implementation this way. I can make it to another PR if that's better.
5959e97

Copy link
Member

Choose a reason for hiding this comment

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

Also refactored the tmpdir.py implementation this way. I can make it to another PR if that's better.

Sure!


setattr(item, "rep_" + rep.when, rep)
if your_unique_key not in item.stash:
item.stash[your_unique_key] = {rep.when: rep.passed}
Copy link
Member

Choose a reason for hiding this comment

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

The previous code stored the entire report, I think that's a bit more generally useful, so would keep that instead of storing just passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.
e20dfb6

@@ -895,29 +895,35 @@ here is a little example implemented via a local plugin:

import pytest

your_unique_key = StashKey[Dict[str, bool]]()
Copy link
Member

Choose a reason for hiding this comment

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

your_unique_key is not a very good name. How about phase_report_key, if you choose to store the full report, or phase_result_key if not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

e20dfb6

Yeah but isn't stash shared with all the other plugins and stuff? If that's the case, it should be remarked that the key name should be unique to avoid collision.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but isn't stash shared with all the other plugins and stuff?

The name doesn't matter, every stash and stash key (i.e. StashKey()) pair are unique.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. I was confused about it.

@ykadowak ykadowak requested a review from bluetech November 27, 2022 06:19
if request.node.rep_call.failed:
print("executing test failed", request.node.nodeid)
report = request.node.stash[phase_report_key]
if ("setup" not in report) or report["setup"].failed:
Copy link
Member

Choose a reason for hiding this comment

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

Generally I don't think it's possible to reach here when "setup" not in report is true (but didn't test it), so I'd remove it. Same for "call" not in report (as long as it's in an elif). Regarding "teardown" not in report -- it's either never true or always true, but I'm not sure which one it is off hand.

Copy link
Contributor Author

@ykadowak ykadowak Nov 27, 2022

Choose a reason for hiding this comment

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

This is probably correct on setup and teardown (tested that teadown doesn't exist here).
Updated 9a69170.

But call may not exist like this #10442 (comment) no?

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks!

@bluetech bluetech merged commit f513d33 into pytest-dev:main Nov 27, 2022
@ykadowak ykadowak deleted the updatedoc branch November 28, 2022 13:55
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