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

fix logreport #10496

Closed
wants to merge 26 commits into from
Closed

fix logreport #10496

wants to merge 26 commits into from

Conversation

anb76ru
Copy link

@anb76ru anb76ru commented Nov 14, 2022

Hello! I use the pytest_runtest_logreport method to get subtest reports. A report is generated for when='call', but if the test failed, the report is not generated. I think this is because the log_passing_tests=True attribute is used in the elif report.failed: block. If the test failed, then if when == 'call' we don't step into the code block:

if not self.log_passing_tests:        
    reporter.write_captured_output(report)

and the report is not generated. I don't understand why self.log_passing_test using in report.failed. Maybe this is a mistake. If you remove the 'if not self.log_passing_tests: ', then the report for the failed tests for when='call' is generated without problems.

I suggest making reporter.write_captured_output(report) unconditional

##10491

OS: win10
pytest_ver: #7.2.0
log_passing_tests

Copy link
Member

@nicoddemus nicoddemus 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 fix does make sense.

I also noticed that write_captured_output should be called in the start of the function, in the if report.passed/if report.when == "call", otherwise passing tests won't output the captured output even if log_passing_tests is True.

We also need tests and a changelog entry. 👍

1. add call write_captured_output for condition report.passed
2. add info into changelog
This reverts commit 5bee912.
@anb76ru
Copy link
Author

anb76ru commented Nov 15, 2022

@nicoddemus , if write_captured_output call without condition if not self.log_passing_tests, than
get failed test test_avoid_double_stdout.
result_test_avoid_double_stdout

Therefore, I moved the condition if not self.log_passing_tests in if report.passed / if report.when == "call". But I am not sure about the correctness of these changes
Now, all test passed
Also, I added information about changes in file changelog.

Please, check PR

@anb76ru
Copy link
Author

anb76ru commented Nov 22, 2022

@nicoddemus, can you review, please ?

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Therefore, I moved the condition if not self.log_passing_tests in if report.passed / if report.when == "call". But I am not sure about the correctness of these changes

Hmm OK, let's remove that. I think that handling could be done more cleanly, but that's out of scope for this PR.

We still need some other changes though:

  1. Add a test for your change (we don't want it to regress in the future)
  2. Rebase and target main, instead of the 7.2.x branch: we always land bug fixes in main, and do a backport later.

@anb76ru anb76ru changed the base branch from 7.2.x to main November 23, 2022 16:44
@anb76ru anb76ru changed the base branch from main to 7.2.x November 23, 2022 16:45
1. remove call write_captured_output(report) from 'call'
2. add test
1. add call write_captured_output for condition report.passed
2. add info into changelog
This reverts commit 5bee912.
1. remove call write_captured_output(report) from 'call'
2. add test
@anb76ru anb76ru changed the base branch from 7.2.x to main November 28, 2022 19:37
@anb76ru
Copy link
Author

anb76ru commented Nov 28, 2022

@nicoddemus I remove condition if not self.log_passing_tests in if report.passed / if report.when == "call"
add test, and change target branch to main. Review please

@anb76ru
Copy link
Author

anb76ru commented Dec 6, 2022

@nicoddemus, Hello. Please check my changes

@nicoddemus
Copy link
Member

Closing as per #10491 (comment).

Sorry again for the misunderstanding of the use case, and thanks for the effort @anb76ru!

@nicoddemus nicoddemus closed this Dec 7, 2022
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.

None yet

6 participants