asdf icon indicating copy to clipboard operation
asdf copied to clipboard

allow wildcard in asdf_schema_root

Open braingram opened this issue 1 year ago • 4 comments

Description

This PR adds support for starting the asdf_schema_root path with a "wildcard" (*) to allow the pytest plugin to collect schema files when pytest is run with --pyargs.

The sunpy CI currently runs all of it's tests using --pyargs. Even though asdf_schema_root is defined in their pytest.ini no schema tests are run. This is due to the asdf pytest plugin expanding the asdf_schema_root based on the path of the configuration file (in the sunpy case this becomes /home/runner/work/sunpy/sunpy/sunpy/io/special/asdf/resources/) which then fails the startswith check for every schema file (which all start with something like ../../.tox/py310-oldestdeps/lib/python3.10/site-packages/sunpy/). With this PR sunpy can update their asdf_schema_root to */sunpy/io/special/asdf/resources/ which will allow the asdf pytest plugin to collect the schemas for testing.

Checklist:

  • [ ] pre-commit checks ran successfully
  • [ ] tests ran successfully
  • [ ] for a public change, a changelog entry was added
  • [ ] for a public change, documentation was updated
  • [ ] for any new features, unit tests were added

braingram avatar Feb 14 '24 16:02 braingram

Thanks for taking a look.

Here's a link to a current sunpy CI run (py312): https://github.com/sunpy/sunpy/actions/runs/8406954022/job/23021402514#step:10:130 The pytest command is this beast:

pytest -vvv -r fEs --pyargs sunpy --cov-report=xml --cov=sunpy --cov-config=/home/runner/work/sunpy/sunpy/.coveragerc /home/runner/work/sunpy/sunpy/docs --cov-report=xml:/home/runner/work/sunpy/sunpy/coverage.xml -n auto --color=yes

This is run in the local clone of the sunpy repo which leads pytest to pick up the pytest.ini in the repo. The log shows:

rootdir: /home/runner/work/sunpy/sunpy
configfile: pytest.ini

This creates a kind of weird setup where the pytest.ini is used from the repo but all test paths are dependent on the install location like the following:

[gw3] [  0%] PASSED ../../.tox/py312/lib/python3.12/site-packages/sunpy/image/tests/test_transform.py::test_flat 
../../.tox/py312/lib/python3.12/site-packages/sunpy/coordinates/frames.py::sunpy.coordinates.frames.Helioprojective 
../../.tox/py312/lib/python3.12/site-packages/sunpy/image/tests/test_transform.py::test_nan_skimage 

So I think the closest match for the tests you ran (thanks for running those!) would be the

regular install, in the sunpy directory - tests fail to start due to ImportPathMismatchError

since tox runs the following before the tests:

python -I -m pip install --force-reinstall --no-deps /home/runner/work/sunpy/sunpy/.tox/.tmp/package/1/sunpy-6.0.dev327+gd35cee0ce.tar.gz

I just tried the following with asdf-standard (I don't have sunpy checked out on this machine). I think this should illustrate the problem (and be a more minimal example of how this PR will hopefully allow sunpy to integrate schema tests in their CI).

cd asdf-standard  # set cwd to the asdf-standard clone
pip install .  # you may have to uninstall asdf-standard first
pytest --pyargs asdf_standard  # runs no tests
# edit `pyproject.toml` to include `asdf_schema_root = '*/resources/schemas'`
pytest --pyargs asdf_standard  # runs all schema tests

braingram avatar Mar 24 '24 23:03 braingram

That's odd... I noticed that they have tox configured to run the tests from a temporary directory instead of the repo root:

https://github.com/sunpy/sunpy/blob/main/tox.ini#L17-L18

but tox must know to grab the pytest.ini before it does that?

in any case I think you're right that a pattern is the way to go. What do you you think about accepting regex instead of a glob style pattern? It would give people more options if say they have non schema YAML files in their directory tree. That would probably need a new config item, asdf_schema_pattern or what have you.

eslavich avatar Mar 25 '24 02:03 eslavich

That's odd... I noticed that they have tox configured to run the tests from a temporary directory instead of the repo root:

https://github.com/sunpy/sunpy/blob/main/tox.ini#L17-L18

but tox must know to grab the pytest.ini before it does that?

I think it's due to the pytest config file search. Since the temporary directory is in the checked out repo pytest will step "up" the tree, back into the repo root and find pytest.ini (since they don't define a rootdir): https://docs.pytest.org/en/7.1.x/reference/customize.html#initialization-determining-rootdir-and-configfile

in any case I think you're right that a pattern is the way to go. What do you you think about accepting regex instead of a glob style pattern? It would give people more options if say they have non schema YAML files in their directory tree. That would probably need a new config item, asdf_schema_pattern or what have you.

I'll give that one some thought.

braingram avatar Mar 25 '24 13:03 braingram

Well I learnt a lot about how tox and --pyargs interact here, not sure that I wanted to though!

Cadair avatar Mar 26 '24 15:03 Cadair

I'm closing this PR.

The pytest plugin lacks unit tests. I think it makes sense to first consider adding tests for the plugin to verify changes (like those in this PR) don't break other features. As this PR was adding a feature that wasn't requested I don't think it's currently worth the risk.

braingram avatar May 10 '24 16:05 braingram