search

conda-incubator / conda-store

Data science environments, for collaboration. āœØ
https://conda.store
BSD 3-Clause "New" or "Revised" License
143 stars 46 forks source link

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

Closed peytondmurray closed 3 months ago

peytondmurray commented 3 months ago

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

Pull request checklist

netlify[bot] commented 3 months ago

Deploy Preview for conda-store ready!

Name Link
Latest commit 9b5afc5c1f01c89fbc4275659d055b3cce89d907
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.

trallard commented 3 months ago

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

jaimergp commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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?