-
Notifications
You must be signed in to change notification settings - Fork 347
tests: Sync expected stream for Pytest' version #996
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I left a suggestion.
tests/test_manage_py_scan.py
Outdated
if result.errlines: | ||
version_out = result.stderr | ||
else: | ||
# Pytest 7.0+ | ||
version_out = result.stdout | ||
|
||
version_out.fnmatch_lines(["*This is pytest version*"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to use an explicit pytest version check, since then when we drop support for pytest<7 it is much easier to find and remove no longer needed code.
if result.errlines: | |
version_out = result.stderr | |
else: | |
# Pytest 7.0+ | |
version_out = result.stdout | |
version_out.fnmatch_lines(["*This is pytest version*"]) | |
if hasattr(pytest, 'version_info') and pytest.version_info >= (7, 0): | |
version_out = result.stdout | |
else: | |
version_out = result.stderr | |
version_out.fnmatch_lines(["*This is pytest version*"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is better! Thanks @bluetech
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluetech, thank you for the suggestion! I thought about this.
I prefer to not rely on version-check but check for actual behavior. For future cleanup I added the comment in code. But of course, your pov as project's maintainer is higher than my own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
Note: version_info
=> version_tuple
(requires Pytest 7.0+).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @stanislavlevin 👍
b9f9e92
to
0056712
Compare
https://docs.pytest.org/en/7.0.x/changelog.html#breaking-changes: > [pytest#8246](pytest-dev/pytest#8246): --version now writes version information to stdout rather than stderr. Fixes: pytest-dev#995 Signed-off-by: Stanislav Levin <[email protected]>
0056712
to
458d48a
Compare
https://docs.pytest.org/en/7.0.x/changelog.html#breaking-changes:
Fixes: #995