Change setting up temporary directories in `astropy` test runner by eerovaher · Pull Request #17529 · astropy/astropy

eerovaher · 2024-12-10T21:36:26Z

Description

astropy v7.0.0 has a bug that causes the XDG_CACHE_HOME environment variable to have no effect. Instead XDG_CONFIG_HOME controls both the cache and configuration directories. #17514 would restore the correct pre-v7.0.0 behavior, but it failed the double test CI job. That job uses the astropy test runner instead of calling pytest directly, and our test runner sets up temporary directories with the set_temp_cache and set_temp_config context managers, which have priority over the environment variables. The failure in #17514 is therefore specific to our test runner and it is not revealing any mistakes in the regression test or in the relevant code. It would be possible to avoid this problem by complicating the regression test, but that is not a robust solution and it is not needed at all if pytest is called directly. It is our test runner that should be edited to avoid using the aforementioned context managers. Our pytest configuration already uses the environment variables instead of the context managers, so this change also makes our test runner configuration more similar to that.

I don't see any reason why the test runner should set up a temporary cache directory because we configure pytest to do that anyways (it actually creates more than one). It is also not obvious to me why our runner should set up a temporary configuration directory, but a code comment (by @astrofrog) mentions timing concerns. I chose to trust the comment, so I replaced rather than removed set_temp_config.

This pull request must be backported because otherwise backporting #17514 will cause double test CI job failures in the v7.0.x branch.

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

Testing the effect of the `XDG_CACHE_HOME` and `XDG_CONFIG_HOME` environment variables using the `astropy` test runner (like the double test CI job does) did not work because the test runner used `set_temp_cache` and `set_temp_config` context managers, which have priority over the environment variables. The test runner has no need to configure the cache directory because `pytest` is configured to do that anyways. Setting up a temporary configuration directory is required to guarantee that `pytest` uses the temporary `astropy` configuration before its own configuration is read, but `set_temp_cache` should nonetheless be avoided.

github-actions · 2024-12-10T21:36:53Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

neutrinoceros

LGTM. Thanks a lot for the comprehensive writeup ! even being involved in previous discussions it was very helpful for me to get back into this specific context.

neutrinoceros · 2024-12-11T15:17:37Z

astropy/tests/runner.py

                return pytest.main(args=args, plugins=plugins)
+            finally:
+                if orig_xdg_config is None:
+                    os.environ.pop("XDG_CONFIG_HOME", None)


I did a double take on the second argument. I now agree this is the correct thing to do: even if this environment variable is supposed to be defined at this point, we cannot guarantee it is still there, and we certainly do no want to raise an exception within a finally clause.

… `astropy` test runner

…529-on-v7.0.x Backport PR #17529 on branch v7.0.x (Change setting up temporary directories in `astropy` test runner)

pllim · 2024-12-11T18:05:38Z

Thanks! But seems like I didn't get to this soon enough.

I don't see any reason why the test runner should set up a temporary cache directory because we configure pytest to do that anyways (it actually creates more than one)

Where is that done; can you please link?

I think the original reason was to ensure the second run does not accidentally read in cached stuff from first run, but @astrofrog can correct me if I am wrong. __pycache__ is not the same as .astropy/cache if that is what you are referring to.

eerovaher · 2024-12-11T18:38:44Z

astropy/conftest.py

Lines 69 to 76 in abc83b4

    
           # Make sure we use temporary directories for the config and cache 
        
           # so that the tests are insensitive to local configuration. 
        
           os.environ["XDG_CONFIG_HOME"] = tempfile.mkdtemp("astropy_config") 
        
           os.environ["XDG_CACHE_HOME"] = tempfile.mkdtemp("astropy_cache") 
        
           Path(os.environ["XDG_CONFIG_HOME"]).joinpath("astropy").mkdir() 
        
           Path(os.environ["XDG_CACHE_HOME"]).joinpath("astropy").mkdir()

astropy/astropy/conftest.py

Lines 87 to 99 in abc83b4

    
           # Make sure we use temporary directories for the config and cache 
        
           # so that the tests are insensitive to local configuration. Note that this 
        
           # is also set in the test runner, but we need to also set it here for 
        
           # things to work properly in parallel mode 
        
           builtins._xdg_config_home_orig = os.environ.get("XDG_CONFIG_HOME") 
        
           builtins._xdg_cache_home_orig = os.environ.get("XDG_CACHE_HOME") 
        
           os.environ["XDG_CONFIG_HOME"] = tempfile.mkdtemp("astropy_config") 
        
           os.environ["XDG_CACHE_HOME"] = tempfile.mkdtemp("astropy_cache") 
        
           Path(os.environ["XDG_CONFIG_HOME"]).joinpath("astropy").mkdir() 
        
           Path(os.environ["XDG_CACHE_HOME"]).joinpath("astropy").mkdir()

astropy/docs/conftest.py

Lines 14 to 21 in abc83b4

    
           # Make sure we use temporary directories for the config and cache 
        
           # so that the tests are insensitive to local configuration. 
        
           os.environ["XDG_CONFIG_HOME"] = tempfile.mkdtemp("astropy_config") 
        
           os.environ["XDG_CACHE_HOME"] = tempfile.mkdtemp("astropy_cache") 
        
           Path(os.environ["XDG_CONFIG_HOME"]).joinpath("astropy").mkdir() 
        
           Path(os.environ["XDG_CACHE_HOME"]).joinpath("astropy").mkdir()

And if the astropy test runner is used then that will create one more configuration directory.

It is simple to prevent our test runner from creating a configuration directory by invoking pytest directly. It is also simple to avoid executing astropy/conftest.py by running pytest docs and it is equally simple to avoid executing docs/conftest.py by running pytest astropy, but I have not managed to run the tests without the creation of at least two configuration and two cache directories.

From what I can tell having conftest.py create the directories should be enough. I don't really understand why our test runner should also create a configuration directory, but a comment mentions timing concerns, which can be tricky to investigate, so I just trusted the comment instead of looking into this properly.

pllim · 2024-12-11T19:15:45Z

Ah, right... pytest.main does respect conftest.py, theoretically. The test runner existed back when pytest was a single file and was bundled with astropy itself (shocking, isn't it?). So it is very possible that we patched different things over time to the point of accidentally duplicating the mkdtemp stuff.

Is there a way to check if conftest.py (the copy that test runner actually uses in astropy.test() which is probably the one that is astropy/astropy/conftest.py) is shipped in astropy wheel? The only reason I can think of when we have to repeat the mkdtemp in runner code is when user does not have access to that conftest.py when calling astropy.test(). I don't think the wheel building log tells me what files are included.

neutrinoceros · 2024-12-12T07:40:40Z

Is there a way to check if conftest.py (the copy that test runner actually uses in astropy.test() which is probably the one that is astropy/astropy/conftest.py) is shipped in astropy wheel?

Yes: .whl are actually just .zip in disguise, so one can just download any wheel from https://pypi.org/simple/astropy/ and unzip it to see what's inside.
For instance

$ cd /tmp/
$ wget https://files.pythonhosted.org/packages/fa/3a/cf06361685f72b50b3fb69247b95b66a6d2edb1b1947e6403c168f01eecc/astropy-7.0.0-cp313-cp313-win_amd64.whl#sha256=7ecaaeeb017cceb02512d5505c43e162b3e26d951cece2b021e786f4c0af17a7
$ unzip unzip astropy-7.0.0-cp313-cp313-win_amd64.whl
$ ls astropy | grep conftest.py
conftest.py

so yes, it's in there (I checked that it is indeed identical to astropy/astropy/conftest.py)

eerovaher added testing no-changelog-entry-needed backport-v7.0.x labels Dec 10, 2024

eerovaher added this to the v7.0.1 milestone Dec 10, 2024

eerovaher requested a review from astrofrog December 10, 2024 21:36

pllim added Extra CI Run cron CI as part of PR Build all wheels Run all the wheel builds rather than just a selection labels Dec 10, 2024

neutrinoceros approved these changes Dec 11, 2024

View reviewed changes

neutrinoceros merged commit 0983d58 into astropy:main Dec 11, 2024

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Dec 11, 2024

Backport PR astropy#17529: Change setting up temporary directories in…

b331f3a

… `astropy` test runner

meeseeksmachine mentioned this pull request Dec 11, 2024

Backport PR #17529 on branch v7.0.x (Change setting up temporary directories in astropy test runner) #17532

Merged

neutrinoceros mentioned this pull request Dec 11, 2024

Fix configuring astropy cache location #17514

Merged

1 task

eerovaher deleted the test-runner-temp-config branch December 11, 2024 15:35

neutrinoceros added a commit that referenced this pull request Dec 11, 2024

Merge pull request #17532 from meeseeksmachine/auto-backport-of-pr-17…

1eae7bd

…529-on-v7.0.x Backport PR #17529 on branch v7.0.x (Change setting up temporary directories in `astropy` test runner)

eerovaher mentioned this pull request Dec 23, 2024

Reduce duplication of temporary test cache and configuration management #17577

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change setting up temporary directories in `astropy` test runner#17529

Change setting up temporary directories in `astropy` test runner#17529
neutrinoceros merged 1 commit intoastropy:mainfrom
eerovaher:test-runner-temp-config

eerovaher commented Dec 10, 2024

Uh oh!

github-actions bot commented Dec 10, 2024

Uh oh!

neutrinoceros left a comment

Uh oh!

neutrinoceros Dec 11, 2024

Uh oh!

pllim commented Dec 11, 2024

Uh oh!

eerovaher commented Dec 11, 2024

Uh oh!

pllim commented Dec 11, 2024

Uh oh!

neutrinoceros commented Dec 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

eerovaher commented Dec 10, 2024

Description

Uh oh!

github-actions bot commented Dec 10, 2024

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

pllim commented Dec 11, 2024

Uh oh!

eerovaher commented Dec 11, 2024

Uh oh!

pllim commented Dec 11, 2024

Uh oh!

neutrinoceros commented Dec 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants