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

Add shell-style wildcard support to 'testpaths' #9897

Merged
merged 1 commit into from May 24, 2022

Conversation

jparise
Copy link
Contributor

@jparise jparise commented Apr 27, 2022

The implementation uses the standard glob module to perform wildcard
expansion in Config.parse().

This is especially useful for large repositories (e.g. monorepos) that
use a hierarchical file system organization for nested test paths.

src/*/tests

The related logic that determines whether or not to include 'testpaths'
in the terminal header was previously relying on a weak heuristic: if
Config.args matched 'testpaths', then its value was printed. That
generally worked, but it could also print when the user explicitly used
the same arguments on the command-line as listed in 'testpaths'. Not a
big deal, but it shows that the check was logically incorrect.

Now that 'testpaths' can contain wildcards, it's no longer possible to
perform this simple comparison, so this change also introduces a public
Config.ArgSource enum and Config.args_source attribute that explicitly
names the "source" of the arguments: the command line, the invocation
directory, or the 'testdata' configuration value.

class ArgsSource(enum.IntEnum):
"""Indicates the source of the test arguments.

.. versionadded:: 7.2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a guess at the next version in which this might appear.

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Thanks! This seems like an useful thing to have, and I agree it's better to store the source of the arguments instead of trying to infer it later. I added a couple of comments.

src/_pytest/config/__init__.py Outdated Show resolved Hide resolved
src/_pytest/config/__init__.py Outdated Show resolved Hide resolved
src/_pytest/config/__init__.py Outdated Show resolved Hide resolved
src/_pytest/config/__init__.py Outdated Show resolved Hide resolved
testing/test_collection.py Outdated Show resolved Hide resolved
testing/test_collection.py Outdated Show resolved Hide resolved
@jparise jparise force-pushed the testpaths-glob branch 2 times, most recently from 44d1ca1 to 4c47ef8 Compare April 27, 2022 15:15
@@ -244,28 +244,32 @@ def test_testpaths_ini(self, pytester: Pytester, monkeypatch: MonkeyPatch) -> No
pytester.makeini(
"""
[pytest]
testpaths = gui uts
testpaths = */tests
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is a backwards-incompatible change (and thus a problem) if the testpaths with --pyargs doesn't work like before anymore. I don't think this test still tests what it was supposed to (namely, that testpaths are Python modules rather than filesystem paths with --pyargs). See #4405 for context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for flagging that! I think I understand. The testpaths concept is overloaded to both refer to file system locations ("paths") and Python module names, and my change thus far requires testpaths to unconditionally resolve to file system locations.

I think a backwards-compatible solution would apply some additional intelligence to the way I'm processing testpaths. If an entry contains a wildcard, then treat it as a file system path and run it through glob; otherwise, accept it as a literal name that could either be path or a module (to be evaluated by later, existing logic).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I have this working as intended in the latest revision.

While this comment was associated with the test_testpaths_ini function, it's test_collect_pyargs_with_testpaths that needed to be reverted back to its original form. It specifically tests for the case you mention, and it was previously failing before I added the additional "does this path contain glob-style wildcards?" check.

Copy link
Member

Choose a reason for hiding this comment

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

@jparise the real painful problem here is that "testpaths" is in fact only paths, not packages, pyargs was a tacked on hack to also look for tests in modules, its not soundly integrated

i'd love to eventually be able to havea consistent idea of collection roots and test id sources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RonnyPfannschmidt fortunately, I think we've arrived at a safe middle ground for this particular change, which only applies the glob expansion to paths when --pyargs isn't specified, which should be unambiguous.

@jparise jparise force-pushed the testpaths-glob branch 2 times, most recently from 32e1ee0 to d21c831 Compare April 28, 2022 16:39
@The-Compiler
Copy link
Member

Ah yes, I only realize now that I was commenting on the wrong test.

Have you considered to check whether --pyargs is supplied and unconditionally using globbing when it is not? That seems like a more straightforward solution to me. But I've never used --pyargs myself, so perhaps someone else can comment if that's a good idea.

This is especially useful for large repositories (e.g. monorepos) that
use a hierarchical file system organization for nested test paths.

    src/*/tests

The implementation uses the standard `glob` module to perform wildcard
expansion in Config.parse().

The related logic that determines whether or not to include 'testpaths'
in the terminal header was previously relying on a weak heuristic: if
Config.args matched 'testpaths', then its value was printed. That
generally worked, but it could also print when the user explicitly used
the same arguments on the command-line as listed in 'testpaths'. Not a
big deal, but it shows that the check was logically incorrect.

Now that 'testpaths' can contain wildcards, it's no longer possible to
perform this simple comparison, so this change also introduces a public
Config.ArgSource enum and Config.args_source attribute that explicitly
names the "source" of the arguments: the command line, the invocation
directory, or the 'testdata' configuration value.
@jparise
Copy link
Contributor Author

jparise commented Apr 28, 2022

Have you considered to check whether --pyargs is supplied and unconditionally using globbing when it is not? That seems like a more straightforward solution to me. But I've never used --pyargs myself, so perhaps someone else can comment if that's a good idea.

I hadn't considered that, but I should have. 😉

I was under the incorrect impression that --pyargs supported "mixed" arguments (paths and modules). Now that I see it's a distinct mode, I check for that flag explicitly and switch the testpaths behavior accordingly.

@jparise
Copy link
Contributor Author

jparise commented May 3, 2022

@The-Compiler and @RonnyPfannschmidt, thanks for your reviews thus far!

Do you have any other feedback or suggestions that I can incorporate into this feature proposal?

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! Seems fine to me now. @RonnyPfannschmidt what do you think?

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

Looks good to go now

@The-Compiler
Copy link
Member

Well then, let's get it in! Thanks @jparise! 🚀

@The-Compiler The-Compiler merged commit 8ac6dce into pytest-dev:main May 24, 2022
24 checks passed
@jparise jparise deleted the testpaths-glob branch October 25, 2022 16:12
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.

None yet

3 participants