Skip to content

fix(vdev): Add --features with default features for vdev test #17977

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.

Sign up for GitHub

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

Merged
jszwedko merged 1 commit into from
Jul 14, 2023
Merged

fix(vdev): Add --features with default features for vdev test #17977

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions vdev/src/commands/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,14 @@ impl Cli {
let runner = get_agent_test_runner(self.container)?;

let mut args = vec!["--workspace".to_string()];
if let Some(extra_args) = &self.args {
args.extend(extra_args.clone());

if !(self.container || extra_args.contains(&"--features".to_string())) {
let features = platform::default_features();
args.extend(["--features".to_string(), features.to_string()]);
}
Comment on lines -44 to -47
Copy link
Collaborator Author

@jszwedko jszwedko Jul 14, 2023

Choose a reason for hiding this comment

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

The fact that this was in the if let Some(...) seems like a straight-up bug. I'm not sure why it also checked for !container previously. I couldn't find any reasoning for that looking back at the PR that introduced this #15833

Copy link
Contributor

@dsmith3197 dsmith3197 Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed. I'm not sure why !container was there, and I'm not sure why we would want to not set the feature flags if running in a docker container. Is there any harm in keeping it?

Copy link
Collaborator Author

@jszwedko jszwedko Jul 14, 2023

Choose a reason for hiding this comment

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

I think the harm with keeping the !self.container is that it won't correctly add --features when needed when running in a container.

Copy link
Contributor

@dsmith3197 dsmith3197 Jul 14, 2023

Choose a reason for hiding this comment

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

Yeah, I agree that behavior seems unintended 👍🏻

if let Some(mut extra_args) = self.args {
args.append(&mut extra_args);
}

if !args.contains(&"--features".to_string()) {
let features = platform::default_features();
args.extend(["--features".to_string(), features.to_string()]);
}

runner.test(
Expand Down