Skip to content

Feature/raw cargo opts #52

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

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

bazaah
Copy link

@bazaah bazaah commented Jul 25, 2019

Adds a parameter 'test_args_raw' that allows users to pass raw args/options to cargo/libtest.

It also refactors how conditional checks are handled: rather than checking all combinations of bools for each endpoint, it builds up a variable, which is passed to cargo test. This makes the logic easier to extend and reason about.

Finally, the various eq / ne conditionals have their syntax altered:

  • Changed 'true' (string) => true (bool)
  • Swapped arg positions so the correct type coercion occurs

@jonhoo
Copy link
Collaborator

jonhoo commented Jul 25, 2019

Thanks, this is a cool change! I've left some inline notes :)

bazaah added 2 commits July 25, 2019 14:29
Fixed functionality related to '--ignore'... should now run ignored
tests in addition to non-ignored tests
@bazaah
Copy link
Author

bazaah commented Jul 25, 2019

As a side note, is there a pipelines syntax checker?

@jonhoo
Copy link
Collaborator

jonhoo commented Jul 25, 2019

Sadly no, not as far as I'm aware. They have a JSON schema that can be used to validate, but not sure it really helps much with things like debugging templates :( Might be something that's worthwhile to suggest over in the Azure DevOps Developer Community. They're decently responsive there.

To better account for spurious type coercion
Copy link
Collaborator

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

This looks great now! Only thing missing is docs. I'd take particular care to point out that raw_opts must contain -- to separate cargo and libtest arguments if you set it, and that while setting raw_opts, arguments injected by other options (like ignored and single_threaded) are still added.

For writing the docs, this needs documenting both in configuration.md (for the new parameter to stages.yml), and in the entries for test and tests in custom.md.

bazaah added 2 commits July 26, 2019 10:19
test_raw_args => test_arguments
raw_opts => test_arguments
of the 'custom arguments' section to better the syntax limitations
@jonhoo
Copy link
Collaborator

jonhoo commented Jul 29, 2019

I had one thought last night -- do you think that any cargo arguments one passes through this one would also want passed to cargo check or to clippy (which also compiles the crate)?

I also just realized that we need to also pass these arguments to coverage, otherwise the test suite won't run correctly there.

@jonhoo jonhoo self-requested a review July 29, 2019 07:47
Co-Authored-By: Jon Gjengset <[email protected]>
@bazaah
Copy link
Author

bazaah commented Jul 29, 2019

I had one thought last night -- do you think that any cargo arguments one passes through this one would also want passed to cargo check or to clippy (which also compiles the crate)?

I also just realized that we need to also pass these arguments to coverage, otherwise the test suite won't run correctly there.

I'm not sure, I haven't looked into the overlap between them. It might be problem but I haven't run into issues yet.

@jonhoo
Copy link
Collaborator

jonhoo commented Jul 29, 2019

I think it comes down to whether you require those arguments to be passed to compile, or whether they are just "nice to haves". Similarly, for coverage, can tests be run without those additional arguments, or can they not? If they can't then we definitely need to pass the flags to tarpaulin for coverage too. If you can't compile without them, they also need to be passed to cargo check and clippy.

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.

2 participants