Skip to content

[flake8-pytest-style] Test function parameters with default arguments (PT028) - #15449

Merged
MichaReiser merged 5 commits into
astral-sh:mainfrom
InSyncWithFoo:PT028
Jan 13, 2025
Merged

MichaReiser merged 5 commits into
astral-sh:mainfrom
InSyncWithFoo:PT028

Conversation

@InSyncWithFoo

Copy link
Copy Markdown
Contributor

Summary

Resolves #15412.

Test Plan

cargo nextest run and cargo insta test.

@github-actions

github-actions Bot commented Jan 13, 2025 •

Copy link
Copy Markdown
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+95 -0 violations, +0 -0 fixes in 6 projects; 49 projects unchanged)

PlasmaPy/PlasmaPy (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ tests/formulary/test_thermal_speed.py:400:43: PT028 Test function parameter `kwargs` has default argument

apache/airflow (+49 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ helm_tests/airflow_aux/test_container_lifecycle.py:102:54: PT028 Test function parameter `hook_type` has default argument
+ helm_tests/airflow_aux/test_container_lifecycle.py:123:59: PT028 Test function parameter `hook_type` has default argument
+ helm_tests/airflow_aux/test_container_lifecycle.py:168:65: PT028 Test function parameter `hook_type` has default argument
+ helm_tests/airflow_aux/test_container_lifecycle.py:187:64: PT028 Test function parameter `hook_type` has default argument
+ helm_tests/airflow_aux/test_container_lifecycle.py:208:68: PT028 Test function parameter `hook_type` has default argument
+ helm_tests/airflow_aux/test_container_lifecycle.py:41:52: PT028 Test function parameter `hook_type` has default argument
+ helm_tests/airflow_aux/test_container_lifecycle.py:78:45: PT028 Test function parameter `hook_type` has default argument
+ helm_tests/airflow_aux/test_pod_template_file.py:927:84: PT028 Test function parameter `hook_type` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:1029:66: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:243:58: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:253:58: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:263:58: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:332:58: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:350:58: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:362:58: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:373:58: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:402:60: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:412:60: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:445:60: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:473:60: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:640:60: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:654:60: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:686:60: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:809:66: PT028 Test function parameter `initial_batch_size` has default argument
+ providers/tests/amazon/aws/hooks/test_eks.py:819:66: PT028 Test function parameter `initial_batch_size` has default argument
... 24 additional changes omitted for project

apache/superset (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ superset/cli/test_db.py:141:66: PT028 Test function parameter `raw_engine_kwargs` has default argument
+ tests/integration_tests/db_engine_specs/base_engine_spec_tests.py:50:63: PT028 Test function parameter `engine_spec_class` has default argument

pandas-dev/pandas (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ asv_bench/benchmarks/gil.py:39:31: PT028 Test function parameter `num_threads` has default argument
+ asv_bench/benchmarks/gil.py:39:46: PT028 Test function parameter `kwargs_list` has default argument
+ pandas/util/_tester.py:15:41: PT028 Test function parameter `extra_args` has default argument
+ pandas/util/_tester.py:15:68: PT028 Test function parameter `run_doctests` has default argument

zulip/zulip (+4 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ tools/lib/test_server.py:56:34: PT028 Test function parameter `skip_provision_check` has default argument
+ tools/lib/test_server.py:57:26: PT028 Test function parameter `external_host` has default argument
+ tools/lib/test_server.py:58:28: PT028 Test function parameter `log_file` has default argument
+ tools/lib/test_server.py:59:18: PT028 Test function parameter `dots` has default argument

astropy/astropy (+35 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ astropy/io/votable/tests/test_vo.py:950:36: PT028 Test function parameter `test_path_object` has default argument
+ astropy/modeling/tests/test_models.py:47:41: PT028 Test function parameter `amplitude` has default argument
+ astropy/modeling/tests/test_models.py:47:54: PT028 Test function parameter `frequency` has default argument
+ astropy/stats/tests/test_bayesian_blocks.py:10:36: PT028 Test function parameter `rseed` has default argument
+ astropy/stats/tests/test_bayesian_blocks.py:167:35: PT028 Test function parameter `rseed` has default argument
+ astropy/stats/tests/test_bayesian_blocks.py:20:33: PT028 Test function parameter `rseed` has default argument
+ astropy/stats/tests/test_bayesian_blocks.py:35:47: PT028 Test function parameter `rseed` has default argument
+ astropy/stats/tests/test_histogram.py:101:38: PT028 Test function parameter `N` has default argument
+ astropy/stats/tests/test_histogram.py:101:50: PT028 Test function parameter `rseed` has default argument
+ astropy/stats/tests/test_histogram.py:112:43: PT028 Test function parameter `N` has default argument
+ astropy/stats/tests/test_histogram.py:112:55: PT028 Test function parameter `rseed` has default argument
+ astropy/stats/tests/test_histogram.py:172:30: PT028 Test function parameter `N` has default argument
+ astropy/stats/tests/test_histogram.py:172:42: PT028 Test function parameter `rseed` has default argument
+ astropy/stats/tests/test_histogram.py:17:28: PT028 Test function parameter `N` has default argument
+ astropy/stats/tests/test_histogram.py:17:41: PT028 Test function parameter `rseed` has default argument
+ astropy/stats/tests/test_histogram.py:31:31: PT028 Test function parameter `N` has default argument
+ astropy/stats/tests/test_histogram.py:31:44: PT028 Test function parameter `rseed` has default argument
+ astropy/stats/tests/test_histogram.py:60:28: PT028 Test function parameter `N` has default argument
... 17 additional changes omitted for project

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PT028 95 95 0 0 0

@InSyncWithFoo

InSyncWithFoo commented Jan 13, 2025 •

Copy link
Copy Markdown
Contributor Author

is_likely_pytest_test() and is_likely_pytest_test_file() were taken almost verbatim from #15158. The only change is that the former now no longer calls the latter. This is because flake8-pytest-style says a function is a test function when its name starts with test_.

Aside from that change, I think is_likely_pytest_test()'s divergence is for the better. I can't say the same about is_likely_pytest_test_file(), however, so it is currently dead code. What should be done with these two?

@MichaReiser MichaReiser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you. I've a few comments regarding the implementation and I think we should remove the fix.

Comment thread crates/ruff_linter/src/rules/flake8_pytest_style/rules/helpers.rs
Comment thread crates/ruff_linter/src/rules/flake8_pytest_style/rules/helpers.rs Outdated
Comment thread crates/ruff_linter/src/rules/flake8_pytest_style/rules/test_functions.rs Outdated
Comment thread crates/ruff_linter/src/rules/flake8_pytest_style/rules/helpers.rs
@MichaReiser MichaReiser added rule Implementing or modifying a lint rule preview Related to preview-mode features labels Jan 13, 2025
@MichaReiser

Copy link
Copy Markdown
Member

Did you review the ecosystem changes?

@InSyncWithFoo

Copy link
Copy Markdown
Contributor Author

@MichaReiser Yes. There are a couple of false positives. For example:

@pandas-dev/pandas:

def test_parallel(num_threads=2, kwargs_list=None):
    """
    Decorator to run the same function multiple times in parallel.
    ...
    """
    ...

@zulip/zulip:

def test_server_running(
    skip_provision_check: bool = False,
    external_host: str = "testserver",
    log_file: str | None = None,
    dots: bool = False,
) -> Iterator[None]:
    ...

These kinds of false positives can be reduced (the former would no longer be reported) by making is_likely_pytest_test() call is_likely_pytest_test_file(). That would be quite a divergence from upstream, however, as discussed above.

@MichaReiser MichaReiser left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for addressing the feedback.

I'm leaning towards not adding the is this a pytest file just yet because it would introduce the need for a new option for those not using the default file name patterns. We can decide to restrict the rule in the future if this comes up often. The other reason why I'm for deferring is that there are still some false negatives because many of the non-test test functions are defined in files prefixed with test_

@MichaReiser
MichaReiser merged commit 47d0a8b into astral-sh:main Jan 13, 2025
@InSyncWithFoo
InSyncWithFoo deleted the PT028 branch January 13, 2025 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preview Related to preview-mode features rule Implementing or modifying a lint rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement new upstream rule PT028

2 participants