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

MAINT - Mock out call to env solve in test_generate_constructor_installer #833

Merged

Conversation

peytondmurray
Copy link
Contributor

@peytondmurray peytondmurray commented Jun 12, 2024

Partially addresses #830.

Description

Currently, test_generate_constructor_installer accounts for 64% of the time spent on unit tests of conda-store-server. This is because the test checks whether specifications can be built into individual installers using constructor, which in turn asks conda to download repodata.json and solve the environment, which takes a long time and is susceptible to network disruptions. I don't think it's actually necessary for us to solve the environment each time here, since we're not interested in testing conda (or constructor) in this test.

This pull request modifies the test to still produce the input files needed to call constructor, but then mocks out the call to constructor itself, avoiding the environment solve entirely. Some simple checks of the call to constructor are made instead, and some quick verification of the files that are meant to be used as input are made using internal constructor functions.

Other changes

  • Added some type annotations in places that I needed to review to understand the code
  • Fixed a DeprecationWarning for a bad escape sequence in a regex that I came across

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

Copy link

netlify bot commented Jun 12, 2024

Deploy Preview for conda-store ready!

Name Link
🔨 Latest commit 9b5afc5
🔍 Latest deploy log https://app.netlify.com/sites/conda-store/deploys/6685af2910a2f90008dd2f15
😎 Deploy Preview https://deploy-preview-833--conda-store.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@trallard trallard left a comment

Choose a reason for hiding this comment

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

Thanks @peytondmurray, I had a quick look and this looks good to me, at least from a logic POV.
I do want, however @jaimergp to have a quick look into this just in case there is something missing but otherwise I will go ahead and approve.

@trallard
Copy link
Collaborator

Also this PR needs updating as it has some merge conflicts @peytondmurray

@jaimergp
Copy link
Member

jaimergp commented Jul 1, 2024

The mocking is solid here and lgtm.

As a side note, I think we are dealing with a symptom of something bigger: why are we running the solver for constructor? Don't we have a lockfile at this point? I think constructor will still create the environment locally even if passed an explicit lockfile, but if we do have a solved environment, the environment itself can also be used as the input, which is a bit more accurate in terms of bundling what we want to bundle.

@trallard
Copy link
Collaborator

trallard commented Jul 3, 2024

The CI is failing, so I re-ran this in case it was a flaky test run.

As a side note, I think we are dealing with a symptom of something bigger: why are we running the solver for constructor? Don't we have a lockfile at this point?

Ideally, we should have a lockfile at this point. @peytondmurray do you have more insights?

@trallard
Copy link
Collaborator

trallard commented Jul 3, 2024

Hmmm, still failing - per the logs, there is something iffy trying to find conda_store_server._internal.action as conda_store_server.action cc/ @peytondmurray

@peytondmurray
Copy link
Contributor Author

Yep, this isn't a flaky test, it's a bad mock - the mock target changed since merging the privatization PR; should be fixed now.

@peytondmurray peytondmurray merged commit 9b86e42 into conda-incubator:main Jul 4, 2024
27 checks passed
@peytondmurray peytondmurray deleted the 830-optimize-unit-tests branch July 4, 2024 00:01
@peytondmurray
Copy link
Contributor Author

Ideally, we should have a lockfile at this point. @peytondmurray do you have more insights?

I'm not sure exactly how this would work; this action takes a specification (i.e. a python object representing an environment.yml or a conda-lock.yml) and formats that as a recipe (i.e. a construct.yaml and some post-install files) for constructor. Then it actually calls constructor in a subprocess.

The only thing this new test does is parameterize some specifications and some lockfiles, pass them to the underlying action, and mock out the subprocess call to constructor. Doesn't constructor always solve specs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

3 participants