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

Feat/test harness #138

Merged
merged 29 commits into from
Jun 26, 2023
Merged

Feat/test harness #138

merged 29 commits into from
Jun 26, 2023

Conversation

tdejager
Copy link
Contributor

@tdejager tdejager commented Jun 23, 2023

This adds the ability to write integration tests, that do actual pixi commands to run the integration test. A PixiController and convenience methods have been added to test these things. Also the integration tests can become pretty expensive so you can use the slow_integration_tests feature to only run these in CI by default. On the commandline one can use cargo t --all-features to test it manually,.

Before merge:

After merge:

  • Make a builder to easily build RepoData @baszalmstra will make a seperate PR and we can use that to test more advanced features a bit faster.

@@ -112,10 +117,14 @@ pub async fn execute(args: Args) -> anyhow::Result<()> {
added_specs.push(spec);
}

// TODO why is this only needed in the test?
Copy link
Member

Choose a reason for hiding this comment

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

is this cleared up now?

@wolfv
Copy link
Member

wolfv commented Jun 23, 2023

Looks good to me! Needs a rebase?

@baszalmstra
Copy link
Contributor

I think it would be nice if you can pass the Args object to the different functions. That would make it easier to call the function with specific optional arguments. E.g. by default you'd do:

pixi.init(Default::default())

but if you want to use some obscure argument you can do:

pixi.init(Args {
  some_obscure_arg: 42,
  .. Default::default()
})

@tdejager
Copy link
Contributor Author

tdejager commented Jun 23, 2023 via email

@tdejager
Copy link
Contributor Author

tdejager commented Jun 23, 2023 via email

@baszalmstra
Copy link
Contributor

I think that can still be achieved through the manifest_path argument. If the passed in args are None the PixiControl overwrites it, otherwise it leaves it intact. The advantage of directly using the Args for the command is that we stay much closer to the interface the CLI command exposes.

Maybe it's also fine if the project is loaded on demand when using PixiControl::project instead of keeping a copy around that you need to update manually for each function you add.

But concretely I ran into this issue because I would like to add a channel argument to the CLI to initialize the project with the specified channel instead of conda-forge. I can now either do this by adding an additional argument to init, which I only rarely need, or by creating another Args struct that is converted to the init::Args or some other convoluted way. It would be much simpler if I could just pass in:

pixi.init(init::Args { channel: .., Default::default() })

Let me know what you think!

@tdejager
Copy link
Contributor Author

Surreee let me give it a go!

@tdejager
Copy link
Contributor Author

One question, regarding the run command with the test harness you want to wait and collect the output and from the CLI interface you never want to do this. So it would be weird to add it to Args. I think.

@tdejager tdejager merged commit ec3121a into prefix-dev:main Jun 26, 2023
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.

4 participants