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

Ask PRs to annotate tests #5694

Closed
wants to merge 1 commit into from
Closed

Conversation

max-sixty
Copy link
Collaborator

  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

As discussed #5690 (comment)

@github-actions
Copy link
Contributor

Unit Test Results

         6 files           6 suites   56m 28s ⏱️
16 210 tests 14 483 ✔️ 1 727 💤 0 ❌
90 456 runs  82 289 ✔️ 8 167 💤 0 ❌

Results for commit e353086.

@@ -1,7 +1,7 @@
<!-- Feel free to remove check-list items aren't relevant to your change -->

- [ ] Closes #xxxx
- [ ] Tests added
- [ ] Tests added, and functions annotated with `-> None:`
Copy link
Member

Choose a reason for hiding this comment

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

Let's discuss this more back in #5690, but I don't think I would understand what this means from the description here alone.

@mathause
Copy link
Collaborator

An alternative would be to use check_untyped_defs. However, this would be much more strict. (I tried that once but ran into mypy errors I could not fix, so I gave up.)

@max-sixty
Copy link
Collaborator Author

An alternative would be to use check_untyped_defs. However, this would be much more strict. (I tried that once but ran into mypy errors I could not fix, so I gave up.)

Ah nice! That would be better. I guess we want it for new code — it would be the herculean effort to fix the old code, most of which doesn't imply a true problem.

FWIW I think most of the false positives are when reassinging variables to different types; e.g. expected =. I guess someone could write a script to increment each new expected = to expected2 = etc, but quite the mission as well.

Is there a way of explaining the TODO in a way that's easy for new contributors to understand? That's probably the closest solution here

@max-sixty max-sixty mentioned this pull request Aug 21, 2021
2 tasks
@max-sixty
Copy link
Collaborator Author

As discussed on the call — can we get mypy coverage?

@dcherian
Copy link
Contributor

@max-sixty
Copy link
Collaborator Author

Nice, thanks @dcherian .

Is anyone familiar with coveralls / whether this can be added? Is it possible to have a rule like "new functions need to be typed", by looking at the diff? That requires storing the previous results.

One other alternative is to do the work to get all the test files typed, and then enforce @mathause 's idea of check_untyped_defs on those files only.

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.

4 participants