Add tests to ensure that context is properly reset

[jaimergp]

Description

See conda/conda#13357

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]

I'll also submit a PR to add a test case (using the add_pip_as_python_dependency: false as an example we can easily check).

[jaimergp]

Awesome! Feel free to push here if you want @mbargull!

[mbargull]

Sorry for the commit notification spam 🙈 .

So, the added test does not fail although it should...
I don't actually know why (additional caching someplace else? IDK).

My plan was to add that test with an xfail for
context.solver == "libmamba" and VersionSpec("<FUTURE_FIXED_VERSION").match(conda.__version__)
in a new PR.
But it's not failing as expected yet 😞.

[jaimergp]

conda-forge/stackvana-feedstock#158 does work though :D

[mbargull]

conda-forge/stackvana-feedstock#158 does work though :D

Oh, yeah, it definitely fixes the issues.

I just want to make sure we have some kind of regression test (esp. since the code around this is pretty finicky).

Now, the issue with the test was that is passed whenever any test has been run before (could have even been the same test for which then only the first invocation failed).
Turns out the code around these stack_context functions has little actual functionality and is just made overly complex for what it is -- and, surprise, it's broken.
The ._last_* members are not properly reset in the .pop (& .replace) function:
https://github.com/conda/conda/blob/23.10.0/conda/base/context.py#L1861-L1866 .
Hence, context is not properly reset.

AFAICT, conda.plan is the only consumer of that code.
So, ideally, we can just deprecate and remove that with a short grace period.
Meaning, we don't have to fix it (would actually just be self._stack[self._stack_idx].apply() -> self.apply()) but can just let it be until removal.

[jaimergp]

So we don't need stack_callback and can just use callback as per conda/conda@c2977d7, right? IOW:

1. We mark Use callback=reset_context in conda.plan conda#13357 as ready for review and wait for it to be merged.
2. We revert the patch in tests.yml
3. We mark this one as ready for review.

[mbargull]

So we don't need stack_callback and can just use callback as per conda/conda@c2977d7, right?

Yes, much simpler and working :).

1. We mark Use callback=reset_context in conda.plan conda#13357 as ready for review and wait for it to be merged.

Done.

2. We revert the patch in tests.yml

Done, and I squashed my debug commit mess and added a xfail so the tests will succeed for current and (expected fixed) conda>23.10.0 future versions.

3. We mark this one as ready for review.

Done.

[jaimergp]

Hm:

FAILED tests/test_api_build.py::test_add_pip_as_python_dependency_from_condarc_file[False] -
Failed: DID NOT RAISE <class 'subprocess.CalledProcessError'>

[mbargull]

The canary ones are okay to fail and will pass once the PR for conda is merged.
The 22.11.0 one is odd -- I'm taking a look.

[mbargull]

Took a look:
For solver=classic we of course need the fixed SubdirData._cache_ mentioned in conda/conda#13357 (comment) .
I'll add the fix to that PR later today or tomorrow morning.

[mbargull]

The tests should fail again now, but succeed whenever conda/conda#13357 is ready and merged.

[jaimergp]

Awesome. Thanks @mbargull!

[jezdez]

Is this still worth adding?

[mbargull]

Is this still worth adding?

Yes.
If we ever get to the point to remove add_pip_as_python_dependency completely, then we can remove this again.
Until then, I'd rather have more than too few tests for this due to the low-level index injections it causes.