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

Display console outputs for all tests #147

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

tfoote
Copy link
Member

@tfoote tfoote commented Sep 6, 2024

Otherwise you can't see console logs of passed tests

Otherwise you can't see console logs of passed tests
@tfoote tfoote requested a review from audrow as a code owner September 6, 2024 18:15
@tfoote
Copy link
Member Author

tfoote commented Sep 6, 2024

This will let us see the output of changes like #146

@rkent
Copy link
Contributor

rkent commented Sep 6, 2024

You did not ask me for a review of this. Perhaps you were only doing this as a test? I don't think it is a good idea if you really want this added, it makes the output too verbose. I can give more commentary if you like.

@tfoote
Copy link
Member Author

tfoote commented Sep 9, 2024

I wanted this because I was reviewing #146 and I can't see the changes in behavior re the warnings. For that example the test runs are basically completely opaque with no console outputs. https://github.com/ros-infrastructure/rosdoc2/actions/runs/10658077327/job/29538644576?pr=146

For automated tests like this being able to go back and find the differences in behavior is valuable. These tests are going to be the only arthives we have of these results. We'll be searching them anyway, and the summaries of the errors and failures still will be generated. This is something that the ROS buildfarm errs on the side of very verbose. But then it helps track things down much faster from failures when they occur, because you can do thiings like compare it to the last know success.

I'd be happy to hear more of your thoughts though.

@rkent
Copy link
Contributor

rkent commented Sep 9, 2024

In the particular PR that you mentioned, the "changed behavior" is that the revised tests succeed, while without the change they fail.

See this run for what happens if you just have the changes to the test, without the change to the core code:

https://github.com/rkent/rosdoc2/actions/runs/10778128703

Isn't that the normal pattern for test-driven development? A reviewer could confirm that by updating the tests without updating the underlying code, which is what the patch does. I can't always guarantee that every change I do will have an associated test, though I try. But #146 does not seem to me to be a good example of the need for more verbosity.

What I would support is using pytest --log-cli-level=WARNING instead. That will show logging warnings, which for some of the other tests are interesting, desirable results.

But that does not help #146. The warning there is buried in conf.py, which for the current implementation is not using the standard logging package. I added a 'warning' there using print(f'[rosdoc2] *** Warning *** which followed the existing practice within that file. We could try to add logging to the messages in conf.py, but I think that once #139 lands, those logging messages are now behind a subprocess again, and I am not sure that pytest will successfully grab them with --log-cli-level. But I have other pending PRs where I have to add warnings to conf.py, as I really want to stop the practice of using raise RuntimeError there which means no documentation at all is produced, so perhaps it would be worth adding standard logging there.

So generally I am not a fan of adding output verbosity for edge cases, as it makes more work in normal cases, and tends to obscure problems more than highlight them. (A particular peeve of mine is the --verbose in the rosindex Makefile, which generates copious amounts of useless output which actually slows the builds. I removed that in my branch, which I will probably propose in the official version as well).

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