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

Annotations in tests, part 1 #725

Merged
merged 7 commits into from Dec 23, 2021
Merged

Annotations in tests, part 1 #725

merged 7 commits into from Dec 23, 2021

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Dec 22, 2021

Draft for now while CI runs.

I started by adding annotations to tests.utils like I did the rest of the project, using the same strict config as before. Then I took a few modules and ran mypy on them, and dealt with any unexpected complaints reported by mypy. Then I suppressed the uninteresting errors in mypy config.

Its complaints so far:

  • test functions aren't annotated. For simple functions this is just disallow_untyped_defs being unhappy that we haven't marked test function as -> None. There are some parameterised tests though, and I found it useful to add in annotations here to ensure the test body was doing something sensible.
    • FWIW I wouldn't be against adding the -> None
  • decorators from pytest and hypothesis return functions whose type involves Any. Not interesting; ignored in config.
  • Mypy spotted that crypto_scalarmult_BYTES and SCALARBYTES were untyped, see 10434eb. Easy to fix.
  • Also in 10434eb, we were calling crypto_box with a string nonce. I'm a bit surprised this worked, to be honest. I guess the fact that the nonce was ascii-encodable (i.e. just nulls) might have helped?

I didn't feel like annotating check_type_error three times, so I pulled it out to tests.utils. It's a bit naughty to do so in this PR (could spin it out?) and I felt a bit icky making utils pull in pytest.

@DMRobertson DMRobertson marked this pull request as ready for review December 22, 2021 19:44
Copy link
Member

@reaperhulk reaperhulk left a comment

Choose a reason for hiding this comment

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

I'm okay with utils having pytest since it's in the tests package 😄

@@ -285,6 +323,7 @@ def test_box_failed_decryption(
def test_box_wrong_length():
with pytest.raises(ValueError):
PublicKey(b"")
# TODO: should the below raise a ValueError?
Copy link
Member

Choose a reason for hiding this comment

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

A good one to look at in the future, but fine to preserve behavior for now.

@reaperhulk reaperhulk merged commit 6f732e6 into pyca:main Dec 23, 2021
@reaperhulk
Copy link
Member

Huge thanks for working through this big project with me. I think this is the last thing I would consider a blocker for the next pynacl release, so now it's just some housekeeping and getting a release out the door in the next few days/weeks.

@DMRobertson
Copy link
Contributor Author

Huge thanks for working through this big project with me.

My pleasure---thanks for reviewing!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants