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

test: add type hinting to test_lib #1012

Merged
merged 7 commits into from
Sep 25, 2023

Conversation

tonyandrewmeyer
Copy link
Contributor

Extends the type hinting for tests:

  • Stop testing the debug level log output (this also removes the need for the logassert package)
  • Pass a dummy spec object rather than None to avoid type errors
  • Ignore type checks when the purpose of the test is to test what happens when bad types are used
  • Minor type hinting where required for pyright to be happy

Partially addresses #1007.

* Stop testing debug level logs.
* Ignore the type checking when we are deliberately checking for bad type handling.
* Pass a dummy spec rather than None, to avoid type complaints.
* Work around an optional argument.
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Very good work, and thanks for the removal of logassert as discussed. No need for this to be a draft PR I think.

@benhoyt
Copy link
Collaborator

benhoyt commented Sep 25, 2023

Looks like there's one failing test on Python 3.8, presumably due to assertNoLogs being added later. So I guess we'll have to avoid that method.

@tonyandrewmeyer
Copy link
Contributor Author

tonyandrewmeyer commented Sep 25, 2023

@benhoyt this failed in CI because unittest.TestCase.assertNoLogs is new in 3.10. Would you just get rid of the check that there's no warning logged, or do a temporary but ugly if hasattr(self, "assertNoLogs") or check that it doesn't happen in a different way?

Edit: oops, Github hadn't refreshed the page, so I didn't notice you already said that. But still, the same question: do you have a preference for how it's avoided? I would tend towards just not doing this check.

@tonyandrewmeyer
Copy link
Contributor Author

tonyandrewmeyer commented Sep 25, 2023

Very good work, and thanks for the removal of logassert as discussed. No need for this to be a draft PR I think.

I changed it to draft when I saw it failed in CI, to hopefully (but not successfully) avoid you bothering with it before I fixed the issue.

@benhoyt
Copy link
Collaborator

benhoyt commented Sep 25, 2023

I changed it to draft when I saw it failed in CI, to hopefully (but not successfully) avoid you bothering with it before I fixed the issue.

Ah, don't worry about that.

Would you just get rid of the check that there's no warning logged, or do a temporary but ugly if hasattr(self, "assertNoLogs") or check that it doesn't happen in a different way?

I think if it's worth doing the test it's worth doing it on Python 3.8 too (a version we need to support). So let's change it to use assertLogs or similar approach that works on 3.8+.

@benhoyt benhoyt marked this pull request as ready for review September 25, 2023 02:57
@benhoyt benhoyt merged commit e054c3f into canonical:main Sep 25, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants