Skip to content

MNT: move pytest.ini configs to .toml #27094

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 23, 2023
Merged

MNT: move pytest.ini configs to .toml #27094

merged 1 commit into from
Oct 23, 2023
+7 −12

Conversation

story645
Copy link
Member

@story645 story645 commented Oct 15, 2023 •
edited
Loading

PR summary

Fixes the pytest asserts not showing using @QuLogic 's suggestion of turning on the import-mode=importlib and moves the contents of pytest.ini into pytest.toml to consolidate since we're not configuring a lot.

PR checklist

@story645 story645 added the CI: testing CI configuration and testing label Oct 15, 2023
@story645 story645 marked this pull request as draft October 15, 2023 05:38
@rcomer
Copy link
Member

rcomer commented Oct 16, 2023 •
edited
Loading

I can reproduce the "partially initialized" errors locally against main with

python -mpytest lib/matplotlib/tests/test_basic.py --import-mode importlib

I note that CI uses python -mpytest

matplotlib/.github/workflows/tests.yml

Line 311 in 112a225

python -mpytest -raR -n auto \

Pytest docs say this about invoking that way.

I also note that the following gives me a bunch of failures that I can't begin to debug

pytest lib/matplotlib/tests/test_backends_interactive.py --import-mode importlib

@story645
Copy link
Member Author

Oh, do we not actually use our .toml file on CI? ☹️

@ksunden
Copy link
Member

ksunden commented Oct 16, 2023

I can confirm locally that pytest ... works but python -m pytest ... fails with this branch checked out.

setting the pythonpath argument in config has no effect. (Didn't really expect it to, but hoped a little bit.)

However running as python -I -m pytest ... seems to elide that problem (-I means "run in isolated mode", ignoring both PYTHON* env vars and not adding the current directory to sys.path), thus it also escapes the "I have PYTHONPATH set" problem, which I also encountered locally (though I will make it so I can just run pytest ... because that is how I prefer it... but that much can be on me)

@ksunden
Copy link
Member

ksunden commented Oct 16, 2023

note that in azure we set PYTHONFAULTHANDLER=1, and -I will ignore this, so we would also want to add -X to enable it via flag instead of envvar. (if we want to keep it)

@QuLogic QuLogic mentioned this pull request Oct 16, 2023
Merged
1 task
@QuLogic
Copy link
Member

QuLogic commented Oct 17, 2023

Might setting -P / PYTHONSAFEPATH work here? python/cpython#31542 I don't have an environment with the new version setup to test right now.

@ksunden
Copy link
Member

ksunden commented Oct 17, 2023

-P helps with the "I have an env var set" part of the problem, but not the "current directory is added to the path" part, which is what is actually causing the main part of the problem when running with python -m

@QuLogic
Copy link
Member

QuLogic commented Oct 17, 2023

But the latter is the point of -P...

@ksunden
Copy link
Member

ksunden commented Oct 17, 2023

Ah, sorry, I had it confused with -E, so I didn't override that part and it still failed... but yeah, -I is essentially just a combination of -E and -P (well, I guess also -s, which disables user site-packages which could actually be problematic in some installs...)...

-E helps with "I have PYTHONPATH set and it fails" -P helps with "Current directory is causing it to fail"

@ksunden
Copy link
Member

ksunden commented Oct 17, 2023

Somehow I get the same error missing either of those two (with PYTHONPATH set)

timhoffm
timhoffm reviewed Oct 20, 2023
View reviewed changes
pyproject.toml Outdated

[tool.pytest.ini_options]
minversion = "7.0"
junit_family = "xunit2"
Copy link
Member

Choose a reason for hiding this comment

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

This is the default nowadays and can thus be omitted:

https://docs.pytest.org/en/7.1.x/reference/reference.html#confval-junit_family

Copy link
Member Author

Choose a reason for hiding this comment

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

so is python_files =test_*py so I'm also gonna omit that
https://docs.pytest.org/en/7.1.x/reference/reference.html#confval-python_files

Copy link
Member

@timhoffm timhoffm Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not exactly.

By default, files matching test_*.py and *_test.py will be considered test modules.

We don't have any *_test.py files under lib (and only one in the whole project: galleries/examples/units/evans_test.py). So, effectively, you'll detect the same files when leaving it out.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't go back in the history, but I'm pretty sure we added python_files first to avoid running evans_test.py, but later we added testpaths = lib, which excluded it in a different manner.

Copy link
Member

Choose a reason for hiding this comment

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

I'm +0.05 on leaving python_files =test_*py to be very explicit what we consider tests. (But OTOH, I likely would not have intoduced it if it was not there).

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, I don't think it needs to stay either.

Copy link
Member Author

Choose a reason for hiding this comment

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

my bias is to not have two configurations that we think are doing the same thing cause I think that'll be harder to debug when everything breaks

@story645 story645 changed the title pytest configs in .toml + importlib MNT: move pytest.ini configs to .toml Oct 20, 2023
@story645 story645 marked this pull request as ready for review October 20, 2023 20:24
QuLogic
QuLogic reviewed Oct 20, 2023
View reviewed changes
pytest.ini
Comment on lines -1 to -3
# Because tests can be run from an installed copy, most of our Pytest
# configuration is in the `pytest_configure` function in
# `lib/matplotlib/testing/conftest.py`. This configuration file exists only to
Copy link
Member

Choose a reason for hiding this comment

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

I think at least the beginning of this comment is important to preserve.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to .toml

@story645 @QuLogic
move pytest configs from .ini to.toml
63a11fa 
remove configs that are now defaults

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@ksunden ksunden merged commit e501543 into matplotlib:main Oct 23, 2023
@story645 story645 deleted the pytest branch October 23, 2023 18:02
@QuLogic QuLogic added this to the v3.9.0 milestone Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@timhoffm timhoffm timhoffm approved these changes

@ksunden ksunden ksunden approved these changes

Assignees
No one assigned
Labels
CI: testing CI configuration and testing Maintenance
Projects
None yet
Milestone
v3.9.0
Development

Successfully merging this pull request may close these issues.
5 participants
@story645 @rcomer @ksunden @QuLogic @timhoffm