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: fix type hint warnings in test_private tests #1010

Merged
merged 6 commits into from
Sep 22, 2023

Conversation

tonyandrewmeyer
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer commented Sep 22, 2023

It looks like there's an accidental double-call in these two tests. They still pass, since the ValueError isn't caught in the second call, but that means that the second call isn't doing anything at all. It would also be weird if the code changed and allowed the values being tested because instead of the test failing in the expected way (the assertRaises failing) it would fail because a TypeError would be raised instead of ValueError.

This also makes pyright happy because we're not (theoretically) passing a datetime.datetime when a string is expected.

test_private is otherwise fine:

$ env PYTHONPATH=.tox/static/lib/python3.10/site-packages/ .tox/static/bin/pyright test/test_private.py 
WARNING: there is a new pyright version available (v1.1.317 -> v1.1.327).
Please install the new version or set PYRIGHT_PYTHON_FORCE_VERSION to `latest`

0 errors, 0 warnings, 0 informations 

Partially addresses #1007

We are explicitly testing passing the wrong type here, so want this to be incorrect.
Looking more closely, I don't believe the double call was intended. The test still passes because the inner call raises the ValueError all the way up, but that means the outer call is never actually used, and it's a bit weird to be testing that passing a datetime.datetime doesn't work (and it would raise a TypeError anyway).
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.

Good spotting, thanks.

I reckon we should just combine the addition of typing to test_private into this PR, as it's part of that, and to avoid tooo many tiny PRs.

@tonyandrewmeyer
Copy link
Contributor Author

I reckon we should just combine the addition of typing to test_private into this PR, as it's part of that, and to avoid tooo many tiny PRs.

Nothing else seems needed for test_private (although I've added it into the list in pyproject.toml).

@tonyandrewmeyer
Copy link
Contributor Author

That rtd fail looks transient. Will re-trigger.

@benhoyt benhoyt merged commit 89d8923 into canonical:main Sep 22, 2023
18 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the pyright-test_private-1007 branch September 22, 2023 06:04
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