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

WP5 indiv process testing: code style improvements #32

Closed
wants to merge 2 commits into from

Conversation

soxofaan
Copy link
Member

This is rebased version of PR #21 after it got in messy conflict with main branch

soxofaan and others added 2 commits January 19, 2024 22:33
- avoid in-place dict mutations
- add type hints
- use more keyword args for call robustness
- more clearly mark internal helpers as private
- finetune exception handling
- finetune pytest asserts (where default value rendering should be good enough)

Rebased version of PR #21
@m-mohr
Copy link
Member

m-mohr commented Jan 19, 2024

I can confirm that #21 and #32 do the same, so #21 can be closed.

Nevertheless, the missing warnings are still a thing. See #21 (comment)

Otherwise lgtm

@soxofaan
Copy link
Member Author

about the missing warnings issue, I did some digging:

warnings.warn

  • only shown in CLI output, not present in HTML report
  • shows up for both passing and failing tests
  • has a feature to suppress repeated warnings, which might be confusing when trying to connect certain warnings with tests that actually triggered it

log.warning

  • shows up in both CLI output and HTML report
  • by default, only shown in CLI output for failing tests, but can be enabled for passing tests too (e.g. --log-cli-level=WARN)
  • in HTML report shown for both failing and passing tests

I think in general log.warning should be prefered over warnings.warn.

@soxofaan
Copy link
Member Author

I'll go ahead and merge this, because there is too much risk on conflicts with other issues/PRs I want to work on

@m-mohr
Copy link
Member

m-mohr commented Jan 22, 2024

I can't confirm this. From what you've stated above, I'd have assumed that adding --log-cli-level=WARN would add the missing warnings back to the log, but it doesn't. They are still missing and we still don't understand why this is the case, so this PR doesn't accomplish what it suggests: Code style improvements (only).

@m-mohr
Copy link
Member

m-mohr commented Jan 22, 2024

Okay, I see what happens here:
Instead of showing up in the warnings summary:
grafik

They now show up in the live log setup:
grafik

That is confusing. They are also not identified as warnings by pytest. Is there a way to fix this, @soxofaan ?

@m-mohr
Copy link
Member

m-mohr commented Jan 22, 2024

Moved the issue to #33 to unblock this PR.

@m-mohr m-mohr self-requested a review January 22, 2024 10:46
Copy link
Member

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

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

Approving with the caveat of the open issue in #33

@soxofaan
Copy link
Member Author

thanks

merged in 0e7a7d7

@soxofaan soxofaan closed this Jan 22, 2024
@soxofaan soxofaan deleted the indiv-process-tests-code-style-tweaks2 branch January 22, 2024 10:57
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