Skip to content

Add hook to check test file names#252

Draft
jl-wynen wants to merge 1 commit intomainfrom
check-test-file-names
Draft

Add hook to check test file names#252
jl-wynen wants to merge 1 commit intomainfrom
check-test-file-names

Conversation

@jl-wynen
Copy link
Member

Currently, this hook fails with

check test file name convention..........................................Failed
- hook id: check-test-file-name
- exit code: 1

  Bad test file name: packages/essnmx/tests/mcstas/mcstas_description_examples.py
  Bad test file name: packages/essreduce/tests/scripts/test_grow_nexus.py

I think the first one needs to be moved or inlined into the file that imports it. But is that too much of a limitation?

@YooSunYoung
Copy link
Member

Or we can just add those exceptions in the ALLOWED_NAMES ....?

@nvaytet
Copy link
Member

nvaytet commented Mar 17, 2026

I would say the test_grow_nexus.py should for sure be renamed.
It actually contains unit tests.

name: check test file name convention
entry: python tools/check-test-file-name.py
language: python
files: '(tests/.*\.py)$'
Copy link
Member

Choose a reason for hiding this comment

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

Did I understand correctly that it will flag any file that does not end with _test?
Sometimes we may have some code in a module that isn't a test but contains common code for setting up tests? And I think these are not always in the conftest.py?

That's probably the case for the mcstas example thing. Is it always possible to move to a conftest file, or are there cases where we cannot?

Also, do we ever have non-python files in the test/ folder, which could end in something else? Like some data files for testing?

Copy link
Member

Choose a reason for hiding this comment

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

That's probably the case for the mcstas example thing. Is it always possible to move to a conftest file, or are there cases where we cannot?

I guess we can always use the conftest file ...

And then we can accept all non-python files under the tests folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's probably the case for the mcstas example thing. Is it always possible to move to a conftest file, or are there cases where we cannot?

This file is imported by only one other file. So as long as it stays that way, we can move the contents to that importing file.

Also, do we ever have non-python files in the test/ folder, which could end in something else? Like some data files for testing?

The regex will only look for .py files. So anything else won't be checked and therefore will be accepted.

Did I understand correctly that it will flag any file that does not end with _test?
Sometimes we may have some code in a module that isn't a test but contains common code for setting up tests? And I think these are not always in the conftest.py?

Correct. I think in principle, we can always move common code into fixtures. But that does not always make sense.
Looking through our ess* packages, I found one case in loki: common.py. This defines a single function to create a workflow which could be provided as a fixture. We do that in other packages.
And esslivedata has a number of modules in its tests. But they seem to define fixtures. So they could be moved into a conftest.py file. But that file would be fairly large.

@YooSunYoung
Copy link
Member

@jl-wynen I fixed the nmx test #253.
Will you review it and merge it to this branch?

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.

3 participants