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

Fix channels with slashes regression #2926

Merged
merged 6 commits into from
Oct 24, 2023
Merged

Conversation

isuruf
Copy link
Contributor

@isuruf isuruf commented Oct 20, 2023

Regression as mentioned in #2431 (comment)

micromamba/tests/test_create.py Outdated Show resolved Hide resolved
@jonashaag
Copy link
Collaborator

Does this also work with pkgs/main/linux-64::mypkg?

@jaimergp
Copy link
Contributor

Line 667 is testing that, I think

Comment on lines 667 to 668
helpers.create("-n", env_name, "pkgs/main/linux-64::python", "--dry-run")
helpers.create("-n", env_name, "pkgs/main::python", "--dry-run")
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused about this test. Is the first line supposed to succeed and not the second? Should both fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

Both should succeed with identical results, imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the regression we observed that pkgs/main/linux-64::xxx was accepted because the last component of the "channel path" was a known subdir, but pkgs/main failed because main is not a known subdir.

Copy link
Member

Choose a reason for hiding this comment

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

I think the try: finally: would prevent the test to fail when these commands fail, no?

Anyhow, you can omit the os.environ reset. This is handled by pytest fixtures.

@@ -652,6 +652,24 @@ def test_spec_with_channel_and_subdir():
)


def test_spec_with_slash_in_channel(tmp_home, tmp_root_prefix):
env_name = "myenv"
try:
Copy link
Member

Choose a reason for hiding this comment

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

prefer with pytest.raises for negative testing. If the create command succeed, the test will still pass.

@AntoinePrv
Copy link
Member

AntoinePrv commented Oct 24, 2023

I took the liberty to resolve my own comments and to rebase.

@AntoinePrv AntoinePrv merged commit f23e078 into mamba-org:main Oct 24, 2023
23 checks passed
@isuruf
Copy link
Contributor Author

isuruf commented Oct 24, 2023

Thanks @AntoinePrv

isuruf added a commit to isuruf/mamba that referenced this pull request Oct 25, 2023
* Fix channels with slashes regression

* fix formatting

* make the test isolated

Co-authored-by: Antoine Prouvost <AntoinePrv@users.noreply.github.com>

* Add more tests

* Use pytest utilities for negative testing

* Add extra test doc

---------

Co-authored-by: Antoine Prouvost <AntoinePrv@users.noreply.github.com>
JohanMabille pushed a commit that referenced this pull request Oct 26, 2023
* Allow defaults::* spec (#2927)

* return architecture levels for micromamba (#2921)

* return architecture levels for micromamba

* fix formatting

* use function multiversioning only on x86

* fix formatting

* update test

* use __builtin_cpu_supports and check for more features

Co-authored-by: Marcel Bargull <mbargull@users.noreply.github.com>

---------

Co-authored-by: Marcel Bargull <mbargull@users.noreply.github.com>

* Fix channels with slashes regression (#2926)

* Fix channels with slashes regression

* fix formatting

* make the test isolated

Co-authored-by: Antoine Prouvost <AntoinePrv@users.noreply.github.com>

* Add more tests

* Use pytest utilities for negative testing

* Add extra test doc

---------

Co-authored-by: Antoine Prouvost <AntoinePrv@users.noreply.github.com>

* Fix for name change

---------

Co-authored-by: Marcel Bargull <mbargull@users.noreply.github.com>
Co-authored-by: Antoine Prouvost <AntoinePrv@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants