Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[tests] conda_env create tests confuse -f and -n #8468

Merged
merged 1 commit into from Apr 8, 2019

Conversation

javabrett
Copy link
Contributor

When a prefix is passed to run_env_command with Commands.ENV_CREATE it should be passed to -n, not -f. Fixed that and corrected tests calling that helper method.

Noticed while investigating #8192.

…e -n not -f.

Signed-off-by: Brett Randall <javabrett@gmail.com>
@javabrett javabrett requested a review from a team as a code owner April 1, 2019 11:40
Copy link
Contributor

@msarahan msarahan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this. I'm pretty sure you're right, but I'll spend a bit more time double-checking the positional arg stuff.

@@ -83,7 +83,7 @@ def run_env_command(command, prefix, *arguments):
arguments[1:1] = ['-n', prefix]
elif command is Commands.ENV_CREATE: # CREATE
if prefix:
arguments[1:1] = ['-f', prefix]
arguments[1:1] = ['-n', prefix]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should test prefix to see if it is path-like (has slashes, leading periods, is an abspath, etc.). If it is a path, this should be -p. If it's a name, it should be -n.

Copy link
Contributor Author

@javabrett javabrett Apr 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't considered -p. It's also possible I'm not across any overloaded meaning of the terms prefix and name, which are used interchangeably in the code in places.

None of the existing tests make use of -p, so there might be some coverage options there. Hopefully none of the existing tests pass a path-type -n - this arg is positional prefix, or so it seems.

Existing tests are mostly creating by name and file, but important to test that the name takes priority in the created env I guess. Just looked wrong that the prefix was passed to file in this one case, and test callers were bent to fit that.

Eventually these tests need to point out issues in

def detect(**kwargs):
filename = kwargs.get('filename')
remote_definition = kwargs.get('name')

... where remote def and name might be passed in a single kwarg without being able to distinguish between them, causing logic issues in that method.

@msarahan
Copy link
Contributor

msarahan commented Apr 8, 2019

@javabrett has signed the CLA. Thanks for your contribution!

@msarahan msarahan merged commit 4b0c479 into conda:master Apr 8, 2019
@javabrett javabrett deleted the fix-conda-env-cli-tests branch April 8, 2019 20:30
@github-actions
Copy link

Hi there, thank you for your contribution to Conda!

This pull request has been automatically locked since it has not had recent activity after it was closed.

Please open a new issue or pull request if needed.

@github-actions github-actions bot added the locked [bot] locked due to inactivity label Aug 28, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants