-
Notifications
You must be signed in to change notification settings - Fork 168
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
Add tests for micromamba, transmuation and miniforge #503
Conversation
0295cdf
to
4a38a10
Compare
I'm not testing on Windows as micromamba isn't used for Windows+miniforge and I don't have the means to debug it easily. (It seems like the tests deadlock in the CI.) |
For reference here is the deadlocked pipeline for Windows: https://github.com/conda/constructor/runs/5536429721?check_suite_focus=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick review and question about the which use, minor need for fixing the condition.
which_cmd = ["conda", "run", "--name", env_name, "which", "micromamba"] | ||
errored, micromamba_path = _execute(which_cmd) | ||
if errored: | ||
_execute(["conda", "create", "--name", env_name, "--yes", "-c", "conda-forge", "micromamba"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this be checking latest micromamba version? Usually new micromamba releases break something and this would make constructor break too. (Unless micromamba tests itself against constructor)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this be checking latest micromamba version?
Yes it is, though I think it's good to catch those breaks here. As it's maturing I hope micromamba's test suite is becoming good enough to minimise the amount of regressions for constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no test at all in micromamba for constructor which is why there are constant failures in miniforge.
I think it's good to have a test here, but it should be after a constructor test has been added to micromamba.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean testing the behavour constructor
expects rather than running constructor
explicitly. For example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we pin the micromamba version and auto update it via auto update PRs?
@chrisburr is it ok if I resume the work here and commit to your branch or would you rather I open a new PR based on this one? |
@jaimergp Thanks for picking this up! I'm happy for you to do whichever you find easiest 😄 |
This is the promised PR to improve the test coverage, in particular:
.conda
Along the way I found a bug that the CI often tested the latest release from the
defaults
channel rather than the current commit. I've fixed this by settingchannel_priority: strict
.