Fixes bug related to envvars_force_uppercase setting
#14942
Conversation
CodSpeed Performance ReportMerging #14942 will not alter performanceComparing Summary
|
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
There was a problem hiding this comment.
Before the envvars_force_uppercase flag was introduced, conda was by default uppercasing all the env vars. Now that the uppercasing can be switched off, the env vars created and set in activate.py are no longer uppercased automatically (when this flag is set to False) and that causes the reported bug.
I think this PR adds a suitable fix for this bug. 👍🏼
You also parametrized all the existing tests to ensure they pass in both cases:
envvars_force_uppercaseis set toTrue.envvars_force_uppercaseis set toFalse
|
@travishathaway Like we spoke earlier, here's a proposed integration test: travishathaway#4 |
tests/shell/__init__.py
Outdated
| """Safely get environment variable, handling different shell behaviors.""" | ||
| try: | ||
| value = sh.get_env_var(varname, "") | ||
| value = None if "Undefined variable" in value else value |
There was a problem hiding this comment.
Special exception for csh and tcsh.
|
Thanks for those suggestions! I didn't realize we had such nice classes setup already for shell integration tests 🫶. I basically plucked the changes you made and committed them directly to this branch. I had to make a couple updates because of some failing tests, so it would be worth it for your take a another look. |
|
I saw some timeout issues in the test runners 😦. I bumped the timeout up to 60 seconds. If this doesn't work, we might have to rethink our approach here or further debug why the tests are taking so long. |
|
Timeout issues are continuing to happen even after bumping it up to 60 seconds 😭. I'm not sure if bumping this up to a very high number is practical given it would probably significantly increase the time our tests need to complete. |
|
Looks like it's only happening for |
|
Let's remove those tests then |
7362380 to
ddad27d
Compare
|
Reset the pull request to the way it originally was but kept your additional change regarding unsetting environment variables. |
| unset_vars.append(env_var) | ||
| # Apply case conversion for environment variables that need to be unset | ||
| if context.envvars_force_uppercase: | ||
| unset_vars.append(env_var.upper()) |
There was a problem hiding this comment.
if envvars_force_uppercase is set to true, why do we need to .upper()?
There was a problem hiding this comment.
We do that here to ensure it's uppercase. When the envvars_force_uppercase is True everything has to be uppercase. When it's False, we allow the variable's case to be whatever the user has designated.
Description
Closes: #14934
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?Add / update outdated documentation?