Skip to content

Further enhance check_consistent.py #8604

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

Merged
merged 5 commits into from
Aug 26, 2022

Conversation

AlexWaygood
Copy link
Member

Add tests to make sure that:

  • All files in test_cases are .py files.
  • All .py files in test_cases have names starting with test_.
  • All requirements listed in .pre-commit.config.yaml also appear in requirements-tests.txt
  • All requirements listed in .pre-commit.config.yaml have the same version specifier as the ones in requirements-tests.txt

Also rename the check_same_files() function to check_no_symlinks(), which is more accurate these days.

Comment on lines +159 to +161
hook = repo["hooks"][0]
package_name, package_rev = hook["id"], repo["rev"]
package_specifier = SpecifierSet(f"=={package_rev.removeprefix('v')}")
Copy link
Member Author

Choose a reason for hiding this comment

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

This feels slightly fragile; not sure if there's a better way

@Akuli
Copy link
Collaborator

Akuli commented Aug 24, 2022

It seems to me that by convention, test_ means "you can run pytest or unittest or something over this directory". Should we instead enforce the opposite, no files named test_blah in our test cases directory that isn't really tests in the usual sense? Maybe not, because the non-test_ file names will show up in editors, and the test_ prefix avoids confusing them with stubs and scripts?

@AlexWaygood
Copy link
Member Author

It seems to me that by convention, test_ means "you can run pytest or unittest or something over this directory". Should we instead enforce the opposite, no files named test_blah in our test cases directory that isn't really tests in the usual sense? Maybe not, because the non-test_ file names will show up in editors, and the test_ prefix avoids confusing them with stubs and scripts?

Hmm, interesting. I'm not wedded to having them be all be called test_foo.py. I mainly added the test here because it was easy to test for, and @sobolevn brought it up in #8520 and #8521.

I guess I think the most important thing is for us to be consistent in whether we call them test_foo.py or just foo.py.

@AlexWaygood
Copy link
Member Author

If we wanted to establish a naming convention that distinguishes these files from the rest of typeshed, but also make clear that these aren't "tests" in the conventional sense, that you can run with pytest, maybe we could establish a naming convention with suffixes instead. Instead of test_foo.py, foo_tests.py?

@sobolevn
Copy link
Member

Or typecheck_ 🤔

@AlexWaygood
Copy link
Member Author

Or typecheck_ 🤔

Ooh I quite like that (though I'd go for the slightly less verbose check_, e.g. check_pow.py, etc.)

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.

4 participants