Skip to content

support passing filter as test binary arguments#265

Merged
sunshowers merged 6 commits into
nextest-rs:mainfrom
tabokie:test-args
Jun 11, 2022
Merged

support passing filter as test binary arguments#265
sunshowers merged 6 commits into
nextest-rs:mainfrom
tabokie:test-args

Conversation

@tabokie

@tabokie tabokie commented Jun 9, 2022

Copy link
Copy Markdown
Contributor

Adding partial support for specifying test binary arguments like cargo. It can
greatly simplify the transitioning from cargo to nextest.

Signed-off-by: tabokie xy.tao@outlook.com

Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie

tabokie commented Jun 9, 2022

Copy link
Copy Markdown
Contributor Author

@sunshowers Another feature request while migrating from cargo. PTAL~

Signed-off-by: tabokie <xy.tao@outlook.com>
@tabokie tabokie changed the title support passing ignore filter as test binary arguments support passing filter as test binary arguments Jun 9, 2022
@sunshowers

sunshowers commented Jun 9, 2022

Copy link
Copy Markdown
Member

Thanks for the contribution!

Hmm, I'm not totally convinced by the direction of this diff. Could you elaborate on the goal and intended audience for this a bit more? Is it to be able to pass in arguments like --include-ignored after a --? If so, I'm hesitant to introduce this sort of hidden alternative method to control the ignored status. Instead, I'd rather look through the passed in filters, warn on such arguments (and also --exact and --skip once filter expression support is stable) and direct users to the recommended ways instead.

@tabokie

tabokie commented Jun 10, 2022

Copy link
Copy Markdown
Contributor Author

Yes, I understand your concern. Personally I think of it as a first step to support test binary arguments (instead of a temporary workaround for certain filters). Ultimately we might want to push down unsupported test binary arguments to internal calls.

Aside from that, currently my main use case is really to keep the test command consistent with cargo workflow. E.g. cargo $CARGO_TEST_COMMAND --workspace -- --ignored can work for both CARGO_TEST_COMMAND="test" and CARGO_TEST_COMMAND="nextest run"

@sunshowers sunshowers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the response. I do see the use case and after having given it some thought, I think it would be OK to support an extremely limited form of this. --ignored and --include-ignored, test binary literals, and nothing else.

Comment thread cargo-nextest/src/dispatch.rs Outdated
Comment thread cargo-nextest/src/dispatch.rs Outdated
Comment thread cargo-nextest/src/dispatch.rs Outdated
Comment thread cargo-nextest/src/dispatch.rs Outdated
Comment thread cargo-nextest/src/dispatch.rs Outdated
Comment thread cargo-nextest/src/dispatch.rs Outdated
Comment thread cargo-nextest/src/dispatch.rs Outdated
Signed-off-by: tabokie <xy.tao@outlook.com>

@sunshowers sunshowers left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for doing this! A couple more comments and this is ready to land.

Comment thread cargo-nextest/src/dispatch.rs Outdated
Comment thread cargo-nextest/src/dispatch.rs Outdated
Signed-off-by: tabokie <xy.tao@outlook.com>
Comment thread cargo-nextest/src/dispatch.rs Outdated
Comment on lines +470 to +471
} else if s.chars().all(|c| c == '-') {
false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is still not quite there:

  1. This needs to just be -- (a single - doesn't match, nor does ---).
  2. Once this is hit, it should go into a mode where the s.starts_with('-') check is disabled and everything is pushed into the filter.

@sunshowers

Copy link
Copy Markdown
Member

That needless_bool lint suggestion is unhelpful and should be disabled for this block.

Signed-off-by: tabokie <xy.tao@outlook.com>
@sunshowers

Copy link
Copy Markdown
Member

Looks fantastic now, thanks! Going to land this once CI passes -- hopefully get a release out in a few days as well.

Signed-off-by: tabokie <xy.tao@outlook.com>
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