Skip to content

Add test case for #595. #2091

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 27, 2016
Merged

Add test case for #595. #2091

merged 1 commit into from
Nov 27, 2016

Conversation

decentral1se
Copy link
Contributor

@decentral1se decentral1se commented Nov 26, 2016 •
edited
Loading

Something to move the conversation on #595 along.

@davidszotten said in #595:

6954363 changed junitxml to include captured output for passing and skipped tests in the output by default.

So far as I can tell, my test shows that the output of a skipped test is not captured by default.

Is this already fixed or I missed something?

@decentral1se
Copy link
Contributor Author

CI failure related to #2087 (comment).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.819% when pulling ff88a88 on lwm:move-595-along into 33796c8 on pytest-dev:master.

nicoddemus
nicoddemus reviewed Nov 26, 2016
View reviewed changes
testing/test_junitxml.py
result, dom = runandparse(testdir)
assert result.ret == 0
node = dom.find_first_by_tag("testsuite")
assert not node.find_first_by_tag("system-out")
Copy link
Member

Choose a reason for hiding this comment

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

To be safer, perhaps it would be better to also assert that "bar!" doesn't appear on the entire XML text?

@nicoddemus
Copy link
Member

Failure is related to: pypa/setuptools#861.

@nicoddemus
Copy link
Member

Could you improve the test like I commented and rebase on master? That should fix CI.

@decentral1se
Copy link
Contributor Author

Fixed up the test and rebased on master 👍

@decentral1se
Add test case for #595.
6c54756 
This new test proves that reports do not capture stdout
by default when skipped.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.819% when pulling 6c54756 on lwm:move-595-along into a6084ed on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.819% when pulling 6c54756 on lwm:move-595-along into a6084ed on pytest-dev:master.

@nicoddemus
Copy link
Member

So it seems at some point the issue on #595 is no longer a problem. Not sure at which point that happened though.

@nicoddemus
Copy link
Member

git bisect shows that the behavior changed in b71add2, so since 2.9.0.

I say we close the original issue as "fixed" and don't bother with a new flag to restore the previous behavior because so far nobody complained (also it seems the original behavior was an accident rather than intentional).

@decentral1se
Copy link
Contributor Author

I say we close the original issue as "fixed" and don't bother with a new flag

Sounds good to me.

Feel free to do whatever with this test.

@nicoddemus
Copy link
Member

It should be merged, it is only asserting existing behavior. 😁

@nicoddemus nicoddemus merged commit 1e97ea6 into pytest-dev:master Nov 27, 2016
@nicoddemus nicoddemus mentioned this pull request Nov 27, 2016
Closed
@decentral1se decentral1se deleted the move-595-along branch November 27, 2016 21:03
@decentral1se
Copy link
Contributor Author

🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicoddemus nicoddemus nicoddemus left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@decentral1se @coveralls @nicoddemus