Skip to content

DEV: Add name-tests-test to pre-commit hooks #23213

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 17, 2022
Merged

DEV: Add name-tests-test to pre-commit hooks #23213

merged 2 commits into from
Jun 17, 2022

Conversation

matthewfeickert
Copy link
Contributor

@matthewfeickert matthewfeickert commented Jun 7, 2022 •
edited
Loading

PR Summary

Add the 'name-tests-test' pre-commit hook which verifies that test files under lib/matplotlib/tests/ conform to pytest naming conventions. Use the --django option to match the test*.py naming convention.

PR Checklist

Tests and Styling

  • [N/A] Has pytest style unit tests (and pytest passes).
  • [N/A] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

@matthewfeickert
Copy link
Contributor Author

matthewfeickert commented Jun 7, 2022 •
edited
Loading

@greglucas Given that we discussed this on Gitter, this is ready for review as pre-commit.ci is passing. Also

$ pre-commit run name-tests-test --all-files
python tests naming......................................................Passed

Obviously no rush here, and I'm very happy to explain any questions that anyone might have. 👍

@jklymak
Copy link
Member

jklymak commented Jun 7, 2022

If we are going to keep adding to pre-commit, then we need to ask everyone to run pre-commit locally before pushing to CI, which I don't believe we currently do in the dev guide.

@matthewfeickert
Copy link
Contributor Author

matthewfeickert commented Jun 7, 2022 •
edited
Loading

If we are going to keep adding to pre-commit, then we need to ask everyone to run pre-commit locally before pushing to CI, which I don't believe we currently do in the dev guide.

@jklymak I thought that part of the resolution from the discussions starting at #22777 (comment) (most of it linked up for reference here in Issue #22949) was that having pre-commmit.ci turned on wouldn't require everyone to use pre-commit locally if they didn't want to as it would fail out (quickly) with decently explicit logs. While I'm coming from the cultural view that wanting people to use pre-commit locally is a good idea, I believe that the matplotlib dev team consensus was that pre-commit should not be a required part of the dev environment to avoid any barriers to contribution (thought I can't find a reference for this at the moment easily, so I'll defer to you here).

So, while I certainly don't mind editing the dev guide to recommend this, I believe this isn't/wasn't desired. Or are you just suggesting that pre-commit be listed as an optional but recommended dependency in the Additional dependencies for development section of the dev guide (it currently is not)?

@oscargus
Copy link
Member

oscargus commented Jun 7, 2022

Should one make running the rest of the tests dependent on pre-commit passing?

@matthewfeickert
Copy link
Contributor Author

matthewfeickert commented Jun 7, 2022 •
edited
Loading

Should one make running the rest of the tests dependent on pre-commit passing?

While it is possible with GitHub Actions to depend on the completion of another workflow with the workflow_run event I personally don't know how easily/if it can be done for an arbitrary webhook.

The additional complexity that it would add to the CI maintenance is also maybe not worth it when you could just throw in a concurrency block so that a follow up push (from the correction that pre-commit found) automatically cancels the previous running job (if there is one).

@oscargus
Copy link
Member

oscargus commented Jun 7, 2022

No, I do not know either. Not even sure if one can make it depend on e.g. linting which is in a separate file (without moving it to the same).

I think it cancels now (as I quite often to follow-up pushes and try to get quite a few emails that something was cancelled), but if there is a better way one should probably look into that (and confirm that it does cancel now).

@matthewfeickert
Copy link
Contributor Author

I think it cancels now (as I quite often to follow-up pushes and try to get quite a few emails that something was cancelled), but if there is a better way one should probably look into that (and confirm that it does cancel now).

Ah yeah, you're right.

matplotlib/.github/workflows/tests.yml

Lines 2 to 4 in e6e63b4

concurrency:
group: ${{ github.workflow }}-${{ github.event.number }}-${{ github.event.ref }}
cancel-in-progress: true

got added in PR #22919 (:rocket:) which is great.

@matthewfeickert matthewfeickert force-pushed the dev/add-name-tests-test-to-pre-commit branch 3 times, most recently from 23cd6d4 to 547819d Compare June 14, 2022 01:16
@matthewfeickert
Copy link
Contributor Author

@jklymak gentle ping to see if you have follow up thoughts/clarification on recommendations on pre-commit checks.

@jklymak
Copy link
Member

jklymak commented Jun 14, 2022

Sorry, I'm neutral on this - but I did install the hooks locally so I stop pushing bad commits 😄

@matthewfeickert
Copy link
Contributor Author

Sorry, I'm neutral on this

Cool. :) Do you have anything that you'd like to see added to this PR then?

but I did install the hooks locally so I stop pushing bad commits smile

👍 I hope that you are finding the experience okay and hopefully nice. If you have feedback on your experience of installing the hooks and getting going I would be very happy to try to incorporate that as suggested recommendations into the developer docs if that's viewed as helpful.

@jklymak
Copy link
Member

jklymak commented Jun 14, 2022

Installing it was fine. Its just one more thing to install, that's all, and I work on different machines, so I tend to be resistant to too many "extras"

@matthewfeickert matthewfeickert force-pushed the dev/add-name-tests-test-to-pre-commit branch 4 times, most recently from 2561ef3 to 00983ba Compare June 16, 2022 17:50
@matthewfeickert
MNT: Update 'pre-commit-hooks' pre-commit hook to v4.3.0
89eacf5
@matthewfeickert
DEV: Add name-tests-test to pre-commit hooks
ade569e 
* Add the 'name-tests-test' pre-commit hook which verifies that test files
under lib/matplotlib/tests/ conform to pytest naming conventions.
   - Use the '--django' option to match the 'test*.py' naming convention.
@matthewfeickert matthewfeickert force-pushed the dev/add-name-tests-test-to-pre-commit branch from 00983ba to ade569e Compare June 17, 2022 00:52
@tacaswell tacaswell added this to the v3.6.0 milestone Jun 17, 2022
@tacaswell tacaswell merged commit 0fe4b08 into matplotlib:main Jun 17, 2022
@matthewfeickert matthewfeickert deleted the dev/add-name-tests-test-to-pre-commit branch June 17, 2022 02:48
@matthewfeickert matthewfeickert mentioned this pull request Jun 24, 2022
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greglucas greglucas greglucas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@matthewfeickert @jklymak @oscargus @greglucas @tacaswell