feat: Add 'name-tests-test' to pre-commit hooks example

[matthewfeickert]

Resolves #227

• Update pre-commit/pre-commit-hooks example to v4.3.0
• Add the 'name-tests-test' pre-commit hooks which verifies that test files under tests/ conform to pytest naming conventions.
  • Use the --pytest-test-first option to match the test_.*\.py naming convention.

[henryiii]

A few thoughts:

• I think this should actually be --pytest-test-first, we name tests test_*.py. Also, this is literally the example on pytest.org's home page so not sure why --pytest (the default) has it with a test suffix instead. Seems like the default should match pytest, which is either test_*.py or *_test.py, with opt-in for one or the other convention.
• There are valid reasons to name at least directories with other names, not sure if it handles those. They are pretty rare though.

[henryiii]

Hmm, this will still say "tests should end in _test.py" even if you pass a flag.

I was going to write:

Just noticed this since someone is trying to add it to scikit-hep: name-tests-test has --pytest (which is the default) requiring *_test.py, but both pytest.org and pytest's own test suite use test_*.py, which is pytest-test-first and not the default. Seems strange that you can't use --pytest on pytest itself or it's examples. Just an (ignorable) suggestion in case you haven't thought of it (maybe there's a history here): maybe --pytest could support both styles (like pytest's default behavior) and --pytest-test-last could be added so selecting either behavior was opt in?

on Anthony's discord, but then found pre-commit/pre-commit-hooks#712 - it seems this is a very old hook and there is a history here. I'm thinking it might not provide enough benefit to add.

[matthewfeickert]

• I think this should actually be --pytest-test-first, we name tests test_*.py.

Agreed! This alias is actually really new as it was added in pre-commit/pre-commit-hooks#779 (15 days ago) and I learned about and added the name-tests-test commit hook to pyhf in scikit-hep/pyhf#1877 (16 days ago).

Also, this is literally the example on pytest.org's home page so not sure why --pytest (the default) has it with a test suffix instead. Seems like the default should match pytest, which is either test_*.py or *_test.py, with opt-in for one or the other convention.

You'd have to take that up with Anthony. Though he notes in pre-commit/pre-commit-hooks#712 (comment) that

note also that the tool's existence predates pytest's change in convention (allowing both names)

I agree with you though that

maybe --pytest could support both styles (like pytest's default behavior) and --pytest-test-last could be added so selecting either behavior was opt in?

makes more sense as a user.

• There are valid reasons to name at least directories with other names, not sure if it handles those. They are pretty rare though.

Looking at the code in pre-commit-hooks for the name-tests-test hook the directory name tests is never used, so it appears that it is just checking the file names, and not the name of the tests directory.

[matthewfeickert]

Hmm, this will still say "tests should end in _test.py" even if you pass a flag.

That's strange. Using the --django option works for pyhf and we use the test_*.py convention (c.f. scikit-hep/pyhf#1877).

[matthewfeickert]

Also scikit-hep/pyhf#1892 is passing. Did you update https://github.com/pre-commit/pre-commit-hooks to v4.3.0 to get the new aliases?

[henryiii]

The title looks okay, maybe that was from an old version? Okay, I think this is fine.

[matthewfeickert]

Okay, I think this is fine.

Thanks for the review. 🙂 In your opinion is this fine to merge, or for the website and recommendations do you want second sign-offs from another member?

[henryiii]

No, one's fine IMO.