Merge pytest addopts #16614

erjac77 · 2022-08-23T16:08:22Z

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

jsirois · 2022-08-23T16:10:18Z

src/python/pants/backend/python/goals/pytest_runner.py

@@ -311,14 +313,32 @@ async def setup_pytest_for_target(
            f"--cov-config={coverage_config.path}",
            *cov_args,
        ]
+        pants_add_opts.extend(["--cov-report", "--cov-config"])  # --cov not there as multi-allowed.
+
+    # Get extra PYTEST_ADDOPTS env var from each possible sources.


This was all starting to feel a bit like too much work / too brittle / etc. so I asked why are we doing things this way / is there another way?

As to why, I traced history and can find no specific justification for choosing env var vs directly using args:

origin of PYTEST_ADDOPTS: Use color in v2 pytest #8901

use of PYTEST_ADDOPTS to deal with junit xml: Add the ability to write junit test results file for pytest runs. #9594

Here's why pytest has to say about PYTEST_ADDOPTS:
https://docs.pytest.org/en/7.1.x/reference/reference.html?highlight=pytest_addopts#envvar-PYTEST_ADDOPTS

The pre-pend bit made me want to know how multiple instance of a flag works then:

python3.9 -mvenv pytest.venv pytest.venv/bin/pip -q install -U pip pytest.venv/bin/pip install pytest==7.0.1 touch test.py $ PYTEST_ADDOPTS="--junit-xml fred.xml" ./pytest.venv/bin/pytest --junit-xml george.xml --junitxml=betty.xml --collect-only -q test.py ------------------------------------------------------------------------------------------------------------------ generated xml file: /home/jsirois/betty.xml ------------------------------------------------------------------------------------------------------------------ no tests collected in 0.00s

So it looks like CLI trumps env var and right-most CLI arg trumps leftmost. Since we completely control the CLI args passed to pytest below, it seems we could just insert the args we require there. Our communication channel then becomes fully robust and looking for duplicates in PYTEST_ADDOPTS to warn is just a nicety at that point. In fact, reading about the -o option, it turns out its a very difficult nicety to unfailingly present! We only check env vars but we could be stomping a setting in config. We've been doing that all along it turns out! So maybe even the dup warnings checking is going to far, or at least we should only be checking for the --junitxml dup, since, FWICT, you cannot specify that option in config.

I'm not sure I follow. There are so many options that the user can pass via PYTEST_ADDOPTS that it becomes difficult to check them all. That's why I simply put a warning and let pytest handle the duplicate options. And what if the user really wants to override an option used/set by pants?

At least, if there is weird behavior like the one I experienced, the user will have a little hint. In my case, I was completely in the dark.

My main point is - can we use argv to pass the args we must have never be over-ridden instead of using the current PYTEST_ADDOPTS mechanism for our own necessary options. That would fully solve robustness.

Then warning about collisions for those options we must pass via the argv stomping user options is just a nicety. I was further pointing out its a nicety we do not extend to junitfamily today since that requires reading all potential config files where users might have set it. Further, doing so would be fragile - we'd be re-implementing the pytest config logic. As such, maybe warning should be jettisoned or at least restricted to just --junit-xml, which, FWICT, cannot be set via config file like junitfamily can.

At least, if there is weird behavior like the one I experienced, the user will have a little hint. In my case, I was completely in the dark.

And you'll still be completely in the dark if you use pytest config files in your repo to set junitfamily. We'll still stomp that silently.

Ok, I see... I'll wait for others to chime in before removing the warnings completely or restricting to only --junit-xml/--junitxml. Thank you for your feedback.

Confirming that I looked at those older PRs and I see no strong reason they used the env var instead of the cli ags, other than the author's personal preference (or them being unware of the args possibility).

jsirois · 2022-08-23T16:12:10Z

src/python/pants/backend/python/goals/pytest_runner.py

+    for popt in pants_add_opts:
+        if any(popt in eopt for eopt in extra_add_opts):
+            logger.warning(
+                f"The option '{popt}' is set by Pants but also in PYTEST_ADDOPTS environment variable. This can cause unexpected behavior."


So, I played around with pytest 7.0.1 which is what main currently ships with by default:

pants/src/python/pants/backend/python/subsystems/pytest.py

Lines 60 to 72 in 9449f42

class PyTest(PythonToolBase):

options_scope = "pytest"

name = "Pytest"

help = "The pytest Python test framework (https://docs.pytest.org/)."

# This should be compatible with requirements.txt, although it can be more precise.

# TODO: To fix this, we should allow using a `target_option` referring to a

# `python_requirement` to override the version.

# Pytest 7.1.0 introduced a significant bug that is apparently not fixed as of 7.1.1 (the most

# recent release at the time of writing). see https://github.com/pantsbuild/pants/issues/14990.

# TODO: Once this issue is fixed, loosen this to allow the version to float above the bad ones.

# E.g., as default_version = "pytest>=7,<8,!=7.1.0,!=7.1.1"

default_version = "pytest==7.0.1"

It has much more variety in what it accepts. FWICT, there are these variants:

--junit-xml or --junitxml

= or

-o or --override-ini

For the last, you currently ignore the -o prefix to junit_family and that's probably fine; so probably this can be ignored. Likewise for 2 you can ignore the introduction of the value separator and you do; so things are still good. For the 1st though, that would need to be checked too.

--junitxml added to the list.

jsirois · 2022-08-23T16:13:58Z

@asherf I added you since you introduced the use of PYTEST_ADDOPTS as a channel for the junit configuration. The channel seems a bit problematic and I was wondering if there was some particular reason you chose it over passing the args via argv.

erjac77 · 2022-08-31T13:19:44Z

Do you think this feature will make its way to 2.12.x (or 2.13)? I don't want to put any pressure, but we are blocked by this bug because it prevents us from properly testing our Django/Nautobot apps.

I suggest to remove the warnings completely and just merge the PYTEST_ADDOPTS environment variables. It would resolve this issue and we can open another ticket to improve the communcation mechanism. What do you think?

jsirois · 2022-08-31T13:31:28Z

It would be good to get additional reviewer feedback, but before requiring that I want to verify your blocked status. Is it not true that for each test where you use PYTEST_ADDOPTS currently you can just add the --junitxml option Pants would have added with a TODO comment that points to this PR? Maybe the problem is you have hundreds of such targets instead of 10s or just a handful?

erjac77 · 2022-08-31T14:10:38Z

How can I determine the resulting filename?

jsirois · 2022-08-31T14:14:26Z

How can I determine the resulting filename?

Two ways:

Derive it by looking at the code. See:

pants/src/python/pants/backend/python/goals/pytest_runner.py

Line 289 in dcba5b2

results_file_name = f"{request.field_set.address.path_safe_spec}.xml"

pants/src/python/pants/build_graph/address_test.py

Lines 338 to 425 in dcba5b2

	def test_address_spec() -> None:
	def assert_spec(address: Address, *, expected: str, expected_path_spec: str) -> None:
	assert address.spec == expected
	assert str(address) == expected
	assert address.path_safe_spec == expected_path_spec

	assert_spec(Address("a/b"), expected="a/b:b", expected_path_spec="a.b")

	assert_spec(Address("a/b", target_name="c"), expected="a/b:c", expected_path_spec="a.b.c")
	assert_spec(Address("", target_name="root"), expected="//:root", expected_path_spec=".root")

	assert_spec(
	Address("a/b", generated_name="generated"),
	expected="a/b#generated",
	expected_path_spec="a.b@generated",
	)
	assert_spec(
	Address("a/b", generated_name="generated/f.ext"),
	expected="a/b#generated/f.ext",
	expected_path_spec="[email protected]",
	)
	assert_spec(
	Address("a/b", target_name="generator", generated_name="generated"),
	expected="a/b:generator#generated",
	expected_path_spec="a.b.generator@generated",
	)
	assert_spec(
	Address("a/b", target_name="generator", generated_name="generated/f.ext"),
	expected="a/b:generator#generated/f.ext",
	expected_path_spec="[email protected]",
	)
	assert_spec(
	Address("", target_name="root", generated_name="generated"),
	expected="//:root#generated",
	expected_path_spec=".root@generated",
	)

	assert_spec(
	Address("a/b", relative_file_path="c.txt", target_name="c"),
	expected="a/b/c.txt:c",
	expected_path_spec="a.b.c.txt.c",
	)
	assert_spec(
	Address("", relative_file_path="root.txt", target_name="root"),
	expected="//root.txt:root",
	expected_path_spec=".root.txt.root",
	)
	assert_spec(
	Address("a/b", relative_file_path="subdir/c.txt", target_name="original"),
	expected="a/b/subdir/c.txt:../original",
	expected_path_spec="a.b.subdir.c.txt@original",
	)
	assert_spec(
	Address("a/b", relative_file_path="c.txt"),
	expected="a/b/c.txt",
	expected_path_spec="a.b.c.txt",
	)
	assert_spec(
	Address("a/b", relative_file_path="subdir/f.txt"),
	expected="a/b/subdir/f.txt:../b",
	expected_path_spec="a.b.subdir.f.txt@b",
	)
	assert_spec(
	Address("a/b", relative_file_path="subdir/dir2/f.txt"),
	expected="a/b/subdir/dir2/f.txt:../../b",
	expected_path_spec="a.b.subdir.dir2.f.txt@@b",
	)

	assert_spec(
	Address("", target_name="template", parameters={"k1": "v", "k2": "v"}),
	expected="//:template@k1=v,k2=v",
	expected_path_spec=".template@@k1=v,k2=v",
	)
	assert_spec(
	Address("a/b", parameters={"k1": "v", "k2": "v"}),
	expected="a/b@k1=v,k2=v",
	expected_path_spec="a.b@@k1=v,k2=v",
	)
	assert_spec(
	Address("a/b", generated_name="gen", parameters={"k": "v"}),
	expected="a/b#gen@k=v",
	expected_path_spec="a.b@gen@@k=v",
	)
	assert_spec(
	Address("a/b", relative_file_path="f.ext", parameters={"k": "v"}),
	expected="a/b/f.ext@k=v",
	expected_path_spec="a.b.f.ext@@k=v",
	)

Observe it by temporarily removing your own PYTEST_ADDOPTS (tests will of course fail) and running Pants with -ldebug which prints out the full environment and argv of every process Pants runs.

Once you've figured the answer out for 1 target via one of the techniques above, the rest should be obvious by pattern.

erjac77 · 2022-08-31T14:57:57Z

Sorry, my question is poorly worded. I know what the name of the xml files will be, but how/where do I specify them? In my case, I only have one BUILD file in the root of my Django/Nautobot tests folder, with sources=["**/test_*.py"] specified in the python_tests target.

If I understand correctly, I would have to create a BUILD file per directory and a pytest_tests target per test, add the --junitxml option to PYTEST_ADDOPTS and duplicate my dependencies and extra_env_var? And what about the coverage? I'll need to add the --cov, --cov-report and --cov-config options too? That's seem a tedious task...

jsirois · 2022-08-31T15:09:49Z

If I understand correctly, I would have to create a BUILD file per directory ...

Ok, so then you do have the issue of "Maybe the problem is you have hundreds of such targets instead of 10s or just a handful?".

I'd really like to avoid ramming this through without proper review / thrashing later to get things right. Currently most Pants maintainers and all the reviewers on this PR are engrossed in a sprint to ship local Pants process execution via docker containers; so I'm trying hard to unblock you elsewise.

Towards that end, see here: https://www.pantsbuild.org/docs/reference-pytest#section-config-discovery
That means you can use pytest.ini which supports specifying addopts there: https://docs.pytest.org/en/7.1.x/reference/customize.html

Does that work for you?

erjac77 · 2022-08-31T15:29:55Z

Ok, so then you do have the issue of "Maybe the problem is you have hundreds of such targets instead of 10s or just a handful?".

I only have one target, but if a want to specify the --junitxml option, I don't know how to do it without duplicating the targets and BUILD files... Maybe I'm missing something?

...so I'm trying hard to unblock you elsewise.

Thank you for that, it is very appreciated!

Concerning pytest config discovery, I don't think it will work because I have many non-Nautoboty tests.

jsirois · 2022-08-31T16:02:21Z

It should work. If the pytest.ini file is in the root of your repo it should automatically apply to all tests. Please give it a quick try and report back.

erjac77 · 2022-08-31T16:19:56Z

Yes, it works for my Nautobot tests, but now all my other tests are failing, because they are not able to load the pytest_nautobot plugin... I guess I could add it in the dependencies of all my tests, but not sure on the long run it's the best solution...

jsirois · 2022-08-31T16:48:24Z

I guess I could add it in the dependencies of all my tests, but not sure on the long run it's the best solution...

Ok, instead of using pytest.ini, just passing extra args as shown below should accomplish the same thing. The below also allows you to add the plugin globally without modifying all test deps - the plugin gets added to the tool instead of to the code under test. This should allow for a self-contained hack I hope that unblocks in a reasonable way unitl folks have time to review this, get it merged and cherry-picked, do releases, etc.

You can add the plugin requirement globally using https://www.pantsbuild.org/docs/reference-pytest#section-extra-requirements

That will force you to use a custom lock file for pytest. The complete setup should be:

[pytest]
# TODO: Remove all this configuration once https://github.com/pantsbuild/pants/pull/16614
# lands and we can upgrade to a Pants version that contains that fix.
args = "-p pytest_nautobot --reuse-db"
# N.B.: You probably want to use a requirement that matches your repo requirement.
# for example "pynautobot==1.1.2".
extra_requirements = ["pynautobot"]
lockfile = "any/path/you/choose/pytest.lock"

You'll need to then run ./pants generate-lockfiles --resolve=pytest. After that you should be able to run any test in your repo and have it pass, whether it uses nautobot or not.

erjac77 · 2022-08-31T20:15:15Z

Well, pytest_nautobot is an internal module (i.e. in the repo) containing a simple pytest hook. Do not confuse with pynautopbot ;) Not sure we can specify a target in [pytest].extra_requirements...

Anyway, I did not have a lot of of test deps to change. Thanks again for your help!

jsirois · 2022-08-31T20:17:14Z

Well, pytest_nautobot is an internal module (i.e. in the repo) containing a simple pytest hook.

In that case can you not just install the hook in an appropriately scoped conftest.py file?: https://docs.pytest.org/en/7.1.x/how-to/plugins.html#requiring-loading-plugins-in-a-test-module-or-conftest-file

erjac77 · 2022-09-04T18:01:37Z

Unfortunately no. I use the pytest_load_initial_conftests bootstraping hook, because I need Nautobot to load its configuration before pytest-django. According to the documentation, this hook is not called for conftest.py files, only for setuptools plugins.

jsirois · 2022-09-30T14:15:50Z

Folks, this has been lingering too long. I enjoined Eric to include / wait on other reviewers to avoid thrashing and I'm hoping @asherf, @Eric-Arellano or @benjyw can review and comment on #16614 (comment) - It is unclear to me why we use PYTEST_ADDOPTS as our internal mechanism of controlling pytest vs just passing args. If one of you could check me on my logic or provide any background on the history, I'd be grateful.

benjyw · 2022-09-30T15:02:24Z

Sorry this has been lingering! I think it makes total sense to:

A) switch the internal use of PYTEST_ADDOPTS to use CLI args.
B) Document that you can add your own pytest opts via PYTEST_ADDOPTS, since we know those won't override the hardwired CLI args we set.
C) (optional) Attempt to warn when a user's PYTEST_ADDOPTS will be ignored because of our hardwired CLI args.

That seems a lot less brittle than attempting to dedup and merge our own PYTEST_ADDOPTS and the user's.

erjac77 · 2022-09-30T20:55:44Z

@jsirois Thanks for the follow up.
@benjyw & @jsirois If it is ok with you both, I'll try to find some time next week to update the pytest backend to use CLI args.

benjyw · 2022-09-30T23:59:10Z

Thanks @erjac77, that sounds great!

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

erjac77 · 2022-10-31T12:40:28Z

@jsirois @benjyw

Sorry for the delay. I finally found the time to work on this issue.

A) switch the internal use of PYTEST_ADDOPTS to use CLI args.

Done. But I was wondering why junit_family is needed since there seems to be an option in from pants.backend.python.subsystems.pytest import PyTest. Maybe someone who knows Pants better can enlighten me? I left it, to be on the safe side.

B) Document that you can add your own pytest opts via PYTEST_ADDOPTS, since we know those won't override the hardwired CLI args we set.

I put the documentation in python_test_goal.md. Let me know if I need to move it somewhere else. Also, do not hesitate to correct me since my english is not very good.

C) (optional) Attempt to warn when a user's PYTEST_ADDOPTS will be ignored because of our hardwired CLI args.

I worked on this one but eventually gave up because there are so many permutations and I couldn't find an elegant way to do these checks.

jsirois

Thanks for sticking with this @erjac77. This looks great to me. A few nits to discuss in documentation. Let me know what you think and then this is ready for merge.

jsirois · 2022-11-01T15:05:53Z

docs/markdown/Python/python-goals/python-test-goal.md

+)
+```
+
+Take note that Pants uses some CLI args for its internal mechanism of controlling Pytest (`--color`, `--junit-xml`, `junit_family`, `--cov`, `--cov-report` and `--cov-config`). In order to preserve the communication channel with Pytest, `PYTEST_ADDOPTS` is not allowed to override these hardwired CLI args.


Well, you are allowed, it's just at your own peril. Since Pants caching is robust, if you tromp one of these that's important (--color is not that important), you'll get a weird failure or unexpected result and you solve by backing out that BUILD change.

So, I think 1st sentence is great, but last should be something like: "Set these at your own peril." or "If these options are overridden, Pants pytest handling may not work correctly."

I made the change, but I thought that CLI args took precedence over env vars.

So it looks like CLI trumps env var and right-most CLI arg trumps leftmost.

Ah, that is right. I did that experiment already: #16614 (comment)
The PR thread is long and my memory is short. That said, your warning verbiage as-is does no real harm I can see. We can always retract it later or re-word to say something more definitive like your customization of these options will be ignored!

jsirois · 2022-11-01T15:09:27Z

docs/markdown/Python/python-goals/python-test-goal.md

@@ -499,3 +499,21 @@ report = true
 ```

 This will default to writing test reports to `dist/test/reports`. You may also want to set the option `[pytest].junit_family` to change the format. Run `./pants help-advanced pytest` for more information.
+
+PYTEST_ADDOPTS environment variable


The title is not too searchable, it assumes you know about this pytest special env var in the 1st place. How about more topical?: "Customize pytest command line options per target"

src/python/pants/backend/python/goals/pytest_runner.py

jsirois · 2022-11-02T13:43:31Z

Thanks again for sticking with this @erjac77. This will naturally flow out into the 2.16.0 release series.

Eric-Arellano · 2022-11-02T14:48:48Z

Thanks @erjac77 ! Great improvement :)

erjac77 added 3 commits August 23, 2022 08:20

Fix PYTEST_ADDOPTS being overriden by the user

ae729df

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Improve alert handling for duplicate addopts

7581c1c

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Check for variant --junit-xml option

dcba5b2

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

jsirois requested review from Eric-Arellano, benjyw, jsirois and asherf August 23, 2022 16:09

jsirois reviewed Aug 23, 2022

View reviewed changes

jsirois added the category:bugfix Bug fixes for released features label Aug 23, 2022

erjac77 added 3 commits October 29, 2022 17:35

Switch internal use of PYTEST_ADDOPTS to use CLI args

8ca8bdc

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Merge remote-tracking branch 'origin/main' into merge-pytest-addopts

7e4e9f7

Override junit_family

9354b43

Document PYTEST_ADDOPTS and add more integration tests

78c4851

jsirois approved these changes Nov 1, 2022

View reviewed changes

Improve docs for the new Pytest CLI args mechanism

b0d1cdd

jsirois merged commit 8fd8960 into pantsbuild:main Nov 2, 2022

	class PyTest(PythonToolBase):
	options_scope = "pytest"
	name = "Pytest"
	help = "The pytest Python test framework (https://docs.pytest.org/)."

	# This should be compatible with requirements.txt, although it can be more precise.
	# TODO: To fix this, we should allow using a `target_option` referring to a
	# `python_requirement` to override the version.
	# Pytest 7.1.0 introduced a significant bug that is apparently not fixed as of 7.1.1 (the most
	# recent release at the time of writing). see https://github.com/pantsbuild/pants/issues/14990.
	# TODO: Once this issue is fixed, loosen this to allow the version to float above the bad ones.
	# E.g., as default_version = "pytest>=7,<8,!=7.1.0,!=7.1.1"
	default_version = "pytest==7.0.1"

Merge pytest addopts #16614

Merge pytest addopts #16614

Conversation

erjac77 commented Aug 23, 2022 • edited by jsirois Loading

jsirois Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

erjac77 Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

erjac77 Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

jsirois Aug 23, 2022

Choose a reason for hiding this comment

jsirois Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

erjac77 Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

benjyw Sep 30, 2022

Choose a reason for hiding this comment

jsirois Aug 23, 2022

Choose a reason for hiding this comment

erjac77 Aug 23, 2022

Choose a reason for hiding this comment

jsirois commented Aug 23, 2022

erjac77 commented Aug 31, 2022

jsirois commented Aug 31, 2022

erjac77 commented Aug 31, 2022

jsirois commented Aug 31, 2022 • edited Loading

erjac77 commented Aug 31, 2022 • edited Loading

jsirois commented Aug 31, 2022

erjac77 commented Aug 31, 2022 • edited Loading

jsirois commented Aug 31, 2022

erjac77 commented Aug 31, 2022 • edited Loading

jsirois commented Aug 31, 2022 • edited Loading

erjac77 commented Aug 31, 2022

jsirois commented Aug 31, 2022

erjac77 commented Sep 4, 2022

jsirois commented Sep 30, 2022

benjyw commented Sep 30, 2022

erjac77 commented Sep 30, 2022 • edited Loading

benjyw commented Sep 30, 2022

erjac77 commented Oct 31, 2022

jsirois left a comment

Choose a reason for hiding this comment

jsirois Nov 1, 2022

Choose a reason for hiding this comment

erjac77 Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

jsirois Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

jsirois Nov 1, 2022

Choose a reason for hiding this comment

jsirois commented Nov 2, 2022

Eric-Arellano commented Nov 2, 2022

erjac77 commented Aug 23, 2022 •

edited by jsirois

Loading

jsirois Aug 23, 2022 •

edited

Loading

erjac77 Aug 23, 2022 •

edited

Loading

erjac77 Aug 23, 2022 •

edited

Loading

jsirois Aug 23, 2022 •

edited

Loading

erjac77 Aug 23, 2022 •

edited

Loading

jsirois commented Aug 31, 2022 •

edited

Loading

erjac77 commented Aug 31, 2022 •

edited

Loading

erjac77 commented Aug 31, 2022 •

edited

Loading

erjac77 commented Aug 31, 2022 •

edited

Loading

jsirois commented Aug 31, 2022 •

edited

Loading

erjac77 commented Sep 30, 2022 •

edited

Loading

erjac77 Nov 1, 2022 •

edited

Loading

jsirois Nov 2, 2022 •

edited

Loading