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

Gazelle considers both test_*.py and *_test.py files to be py_test targets. #1816

Closed
dougthor42 opened this issue Mar 20, 2024 · 4 comments · Fixed by #1819
Closed

Gazelle considers both test_*.py and *_test.py files to be py_test targets. #1816

dougthor42 opened this issue Mar 20, 2024 · 4 comments · Fixed by #1819
Labels
gazelle Gazelle plugin related issues

Comments

@dougthor42
Copy link
Contributor

🐞 bug report

Affected Rule

  • gazelle

Is this a regression?

Nope! Just unexpected behavior.

Description

A typical python project will use only one of *_test.py or test_*.py to name test files.

However, gazelle will make py_test targets for both types:

} else if strings.HasSuffix(f, "_test.py") || strings.HasPrefix(f, "test_") {

if strings.HasSuffix(baseName, "_test.py") || strings.HasPrefix(baseName, "test_") {

This can lead to unexpected behavior:

Example 1: unit test utilities

Assume that a project uses *_test.py for unit/integration tests. Using pytest, the config would be:

[tool.pytest.ini_options]
python_files = ["*_test.py"]

Also assume that there needs to be some utilities related to testing. A typical solution would be to make a test_utils.py file because that does not get collected by the pytest test runner.

gazelle will incorrectly (at least according to the user) make:

py_test(
    name = "test_utils",
    srcs = ["test_utils.py"],
)

The user expects a py_library instead.

Example 2: Non-code tests, such as electrical or physical testing

Code that deals with electrical or physical testing (voltage, current, vibration, pressure, etc) is typically called test_foo or foo_test. For example, code that determines the breakdown voltage of a part might be called test_breakdown_voltage.py. Code might also need to create collections or suites of non-code tests, and would live in test_suites.py.

Again, according to the user, gazelle will incorrectly make py_test targets for such instead of py_library targets.

🔬 Minimal Reproduction

  1. Make test_foo.py
  2. Make foo_test.py
  3. Run gazelle
  4. See that both were generated as py_test targets.

🔥 Exception or Error

N/A

🌍 Your Environment

Operating System:

  
$ lsb_release -a
No LSB modules are available.
Distributor ID: Debian
Description:    Debian GNU/Linux rodete
Release:        n/a
Codename:       rodete
  

Output of bazel version:

  
$ bazel version
Starting local Bazel server and connecting to it...
Build label: 7.1.0
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Mon Mar 11 17:55:31 2024 (1710179731)
Build timestamp: 1710179731
Build timestamp as int: 1710179731
  

Rules_python version:

  
0.31.0 @ bdb2aa2e5c0ca435918037aa3568c0b0c4c9c1ab
  

Anything else relevant?

Don't think so.

@dougthor42
Copy link
Contributor Author

This can be solved by adding a directive (I seem to love directives...). Something like:

# gazelle:python_test_file_pattern *_test.py

I imagine the directive supporting a comma-separated list of globs with the default obviously matching the existing behavior:

# The default.
# N.B.: We probably would want to change the 2nd to include the extension: "test_*.py"
# gazelle:python_test_file_pattern *_test.py,test_*

# only *_test.py will be made into `py_test` targets
# gazelle:python_test_file_pattern *_test.py

# any glob is actually valid.
# matches "A_test_a.py", "A_test_1.py", "B_test_f.py", etc., and the literal "unittest.py"
# gazelle:python_test_file_pattern [A-Z]_test_?.py,unittest.py

If this sounds like a good solution LMK and I'll be happy to take a stab at it.

@wingsofovnia
Copy link
Contributor

It occurred to me today as well and I literally have test_utils.py file :) Is test_*.py even a common convention in Python?

@dougthor42
Copy link
Contributor Author

Is test_*.py even a common convention in Python?

Yes, though it seems like it's going out of style in favor of *_test.py (but that might just be observation/recency bias).

@aignas
Copy link
Collaborator

aignas commented Mar 21, 2024

The suggested implementation sounds good to me. I think the default behaviour even had a few tests to ensure that people can create libraries with 'test' in the file. But that might have changed at some point.

@aignas aignas added the gazelle Gazelle plugin related issues label Mar 29, 2024
github-merge-queue bot pushed a commit that referenced this issue Apr 5, 2024
Add the `python_test_file_pattern` directive. This directive allows
users to configure what python files get mapped to the `py_test`
rule.

The default behavior is unchanged: both `test_*` and `*_test.py`
files generate `py_test` targets if the directive is unset.

The directive supports multiple glob patterns, separated by a comma.

Note: The original code used, effectively, `test_*` for one of the
patterns. This code uses `test_*.py` instead. These are equivalent
because of the `.py` extension check prior to pattern matching.

Fixes #1816.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gazelle Gazelle plugin related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants