pants icon indicating copy to clipboard operation
pants copied to clipboard

Opt-in tags

Open thejcannon opened this issue 3 years ago • 13 comments

Is your feature request related to a problem? Please describe. I'd like to add a manual tag to tests, however tags are currently opt-in so everywhere I invoke test I'll need to also do the filtering.

Describe the solution you'd like I'd like to be able to declare certain tags be opt-out by default

Describe alternatives you've considered I tried setting [GLOBAL].tag=["-manual"] in my pants.toml, but then there seems no way to select the manual targets. The following returns an empty-set ./pants --tag='+manual' lint path/to/manual/tests/::

Additional context N/A

thejcannon avatar Apr 05 '22 20:04 thejcannon

Have a similar situation, where it would be convenient to exclude a target x per default, when running ./pants package :: instead of using ./pants --tag='-x' package ::

alexeckert avatar May 17 '22 07:05 alexeckert

Idea. If the tag is added to the target with a - prefix, it could be treated as opt-out.

python_tests(
  ...,
  tags=["-manual"]
)

Then a normal ./pants test :: should exclude all targets with any -tags, while ./pants --tag=manual ... would include them.

Edit: Or perhaps a different syntax would be clearer, such as tags=["manual!"], where the exclam marks it as an opt-out tag.

kaos avatar May 17 '22 12:05 kaos

The - looks funny because we already use it to mean exclude.

If anything I would expect this to be a pants.toml setting because otherwise it can get really confusing (what if you forget to do the right thing on one target?)


FWIW we actually ended up not needing this. I use pytestmarks = pytest.mark.manual in my manual tests. That way they still get "run" to validate imports and syntax. (Big win)

thejcannon avatar May 17 '22 13:05 thejcannon

FYI I recently fixed several issues with --tag filtering, which was ~prework for this: https://github.com/pantsbuild/pants/pull/15394

As with most Pants feature requests...once we have a design figured out, this will be very trivial to implement :)

Eric-Arellano avatar May 17 '22 13:05 Eric-Arellano

The - looks funny because we already use it to mean exclude.

Yeah, I noticed after posting, hence the edit ;)

But as you point out, Josh, a typo in a targets tag would turn it into a potential hard to find issue..

kaos avatar May 17 '22 14:05 kaos

I'm back on needing this until there's an in-house solution for https://github.com/pantsbuild/pants/issues/15482

thejcannon avatar May 19 '22 17:05 thejcannon

The actual work around is as follows:

# BUILD
python_tests(
    sources = [
        "**/test_*.py",
    ],
    tags = ["test_py3_single"],
)

python_tests(
    name = "test_py3_matrix",
    interpreter_constraints = parametrize(
        py310 = ["CPython==3.10.*"],
        py311 = ["CPython==3.11.*"],
        py39 = ["CPython==3.9.*"],
    ),
    sources = [
        "**/test_*.py",
    ],
    tags = ["test_py3_matrix"],
)
# pants.toml

[GLOBAL]
...
tag = ["-py3_matrix"]
...

and the default is to not run that

Then to run in it CI

pants --tag="['+test_py3_matrix','-test_py3_single']" test ::

rbuckland avatar Apr 28 '23 06:04 rbuckland

On the design front here. I think for the original use case (which I share) of unit vs other type of tests, the opt-in would need to be per goal. That is one would want to exclude from pants test by default, but not fmt, lint, etc.

cburroughs avatar Oct 05 '23 21:10 cburroughs

Going to take a stab at solving this, it led to a fairly confused debugging session today where a target wasn't usable with no meaningful diagnostic.

tgolsson avatar Nov 10 '23 19:11 tgolsson

I did an attempt with the above PR but I think the semantics aren't super-great in terms of composition. While I'm not excluding that route I'm wondering whether either

python_tests(
    ...
    tags=["manual"],
)

or

python_tests(
    ...,
    manual=True
)

might be an interesting approach. This'd take semantics from its bazel namesake, and simply mean "is not part of globs". Simple semantics, and solves a lot of use-cases. Using manual=... might also make it possible to do per-goal overloading, but it might be a bit of an ordering problem as filters are built independently of goals. But it'd definitely be extensible for that in the future, which a tag wouldn't (easily) be.

I'm sure there are edge-cases here that might be hairy -- f.ex. list or filter might want to always include them, and so on. But those cases I think aren't blockers.

Thoughts?

tgolsson avatar Nov 13 '23 20:11 tgolsson

is the word manual significant in these examples? I think that this example:

python_tests(
    ...,
    manual=True
)

may be problematic unless manual is meant to be a registered field.

the former, example:

python_tests(
    ...
    tags=["manual"],
)

what makes this any different from any other tag?

kaos avatar Nov 13 '23 21:11 kaos

It would be a registered field, and I'd envision it being part of the COMMON_FIELDS tuple to make it appear almost everywhere a tags field appears already.

The tag manual would have special semantics. This is what bazel says:

manual keyword will exclude the target from expansion of target pattern wildcards (..., :*, :all, etc.) and test_suite rules which do not list the test explicitly when computing the set of top-level targets to build/run for the build, test, and coverage commands. It does not affect target wildcard or test suite expansion in other contexts, including the query command. Note that manual does not imply that a target should not be built/run automatically by continuous build/test systems. For example, it may be desirable to exclude a target from bazel test ... because it requires specific Bazel flags, but still have it included in properly-configured presubmit or continuous test runs.

Due to the issues discussed in the PR, the exact semantics here are quite hard to achieve without a blessed behaviour or a rework of tags composition.

tgolsson avatar Nov 13 '23 22:11 tgolsson

I think it's a good place for overall design improvement here. Right now to run all tests except the tests with "it" tag I have to run:

pants test --filter-tag-regex="['-it']" ::

What I'd really like to do is to set default filter for test goal. Something like this:

[test.filter]
tag_regex = ["-it"]

This might also be useful for other goals, for example, I could imagine people who don't want to lint test sources specifically. To achieve this you have to run something like this:

pants lint --filter-target-type="-python_test" ::

Instead they could do:

[lint.filter]
target_type = ["-python_test"]

I think it's a pretty generic concept we could use for these goals, maybe even other subsystems.

Another benefit of using pants.toml for this is that we can set one default for development and another default for ci. Lets look at my example with integration tests. I can set in pants.toml:

[test.filter]
tag_regex = ["-it"]

but for pants.ci.toml I can set:

[test.filter]
tag_regex = []

which will reset the filter (right know it doesn't work this way, but we could do that)

pp-gborodin avatar Jan 03 '24 20:01 pp-gborodin