Reduce flakiness of CI test runs

[mbargull]

Description

I started to include unrelated test fixes in #4650 to easier inspect failing/passing of relevant tests.
Those started to grow and it's cleaner to have them separated, anyway, so here we go.

---

What is the main change/result? (See details/motivation below.)

I went back an forth between having a fixture to add config=testing_config either completely implicit (autouse=True) or have it explicitly added to each and every test to make its use obvious.

Now I went the middle ground with pytestmark = pytest.mark.usefixtures("api_default_testing_config") to explicitly request the fixture for a test module but not on the single test level. If one wants to deactivate it for a single test but still use it for all other tests in a module, they can @pytest.mark.no_api_default_testing_config such test.

---

What is the overall motivation and why this back and forth, then middle ground, anyway?

The main cause of the tests' flakiness _{(apart from CRAN connection and occasional pip %TEMP% permission errors on Windows)} is that the parallel tests ought to write to separate locations but in reality most don't.
The mechanism to have them use separate locations is the testing_config fixture which sets Config.croot to a temporary directory.
So nearly all tests should explicitly run function(..., config=testing_config) for all functions that take a config parameter.
But actually, many don't, so they can occasionally run into concurrent write conflicts (see #4653 (comment) ).

Hence, the obvious fix is to add config=testing_config to all those tests.
That results in a large change set with somewhat verbose code, but is also not future-proof (since I expect it'll be forgotten to be added for newer tests again in the future).
The other solution is to have config=testing_config always unconditionally, implicitly used.
The downside with that is that it adds more hidden/non-obvious behavior.

The proposed change is to not go full explicit- but also not full-implicit mode by adding a fixture that can be

• explicitly added to single tests via @pytest.mark.usefixtures("api_default_testing_config"),
• implicitly added to all of a module's tests via pytestmark = pytest.mark.usefixtures("api_default_testing_config"),
• explicitly deactivated for a single test via @pytest.mark.no_api_default_testing_config.

Checklist - did you ...

• Add a file to the news directory (using the template) for the next release's release notes?
• Add / update necessary tests?
• Add / update outdated documentation?

[mbargull]

Comments copied from gh-4652:

I didn't add a news entry because this is just chore that doesn't touch anything outside the test code. If you'd still like to have a news entry, let me know.

Re: config=testing_config: Previously, tests failed because (even when only rendering) concurrent writes/deletions around the {croot}/work happened at conda_build.config.Config.compute_build_id (at the end in its if old_dir != work_dir: branch) happened. I didn't dig into it, but it might just be that another test creates the work (without a timestamp) folder and then compute_build_id wants to move it. In any case, testing_config sets the croot to testing_workdir and as such the single tests use their own root dir without seeing others' work folders.

[mbargull]

(Added description/motivation to the OP.)

[jezdez]

Thank you for working on this @mbargull!

I don't think your approach with rewriting the function defaults for conda_build.api is a good pattern and could be achieved by a more mundane monkey patching of the conda_build.config.get_or_merge_config function, to be automatically returning the appropriate test config.

While I enjoy seeing the use of pytestmark, I agree that it'll likely be forgotten by future contributors when adding new tests, creating rather unpleasant implicit differences in behavior again, increasing the risk of concurrency issues.

I would suggest to indeed use autouse=True to automatically enable the test config, if you think the majority of test modules need it (as it seems) and instead try to find a way to disable it when needed.

[mbargull]

I don't think your approach with rewriting the function defaults for conda_build.api is a good pattern and could be achieved by a more mundane monkey patching of the conda_build.config.get_or_merge_config function, to be automatically returning the appropriate test config.

Haha, and that's what I get to only look at level n but not level n+1 :) -- I didn't even see/look for that all those functions use get_or_merge_config.
So, yes, very much agreed, I'll change it to monkeypatch get_or_merge_config; much cleaner.

I would suggest to indeed use autouse=True to automatically enable the test config, if you think the majority of test modules need it (as it seems) and instead try to find a way to disable it when needed.

Okay, I guess autouse=True is fine since we can opt out via the added @pytest.mark.no_<...>.
Mind you, this would mean that we create a temporary directory for every single test then (through testing_config -> testing_workdir -> tmpdir fixture requests).
We then probably want to set pytestmark = pytest.mark.no_default_testing_config for some test modules like test_api_consistency.py.

[jezdez]

I don't think your approach with rewriting the function defaults for conda_build.api is a good pattern and could be achieved by a more mundane monkey patching of the conda_build.config.get_or_merge_config function, to be automatically returning the appropriate test config.

Haha, and that's what I get to only look at level n but not level n+1 :) -- I didn't even see/look for that all those functions use get_or_merge_config. So, yes, very much agreed, I'll change it to monkeypatch get_or_merge_config; much cleaner.

Oops, conda-build keeps on giving 🤣

I would suggest to indeed use autouse=True to automatically enable the test config, if you think the majority of test modules need it (as it seems) and instead try to find a way to disable it when needed.

Okay, I guess autouse=True is fine since we can opt out via the added @pytest.mark.no_<...>. Mind you, this would mean that we create a temporary directory for every single test then (through testing_config -> testing_workdir -> tmpdir fixture requests). We then probably want to set pytestmark = pytest.mark.no_default_testing_config for some test modules like test_api_consistency.py.

Hmm, yeah, let's see what the impact on runtime is for that

[jezdez]

Seems like it's only one test failing now! test_skeleton_pypi_arguments_work

[mbargull]

Seems like it's only one test failing now! test_skeleton_pypi_arguments_work

Ha! That was a sneaky PyPI update since Saturday 😁 .
I fixed the test to use the same package version as before. The the commit message from c6463d9 for details.

[mbargull]

This is now a much smaller and cleaner change set with which I'm content :).
The test run time is not noticeably affected since we have rather heavy test cases that dominate all other running times.

@jezdez, thanks for your thorough and thoughtful review, I appreciate it!
Let me know if there's anything else you'd want me to change.

[mbargull]

Remaining failure is the permission error on Windows during pip install for peppercorn again.

[kenodegard]

Reran failed test, seems to be passing now

concerns have been resolved

[mbargull]

Looks like

@pytest.fixture(scope="function", autouse=True)
def default_testing_config(testing_config, monkeypatch, request):

doesn't work (reliably/at all?): #4665 (comment)