Skip to content

[nextest-runner] add a --tool-config-file option #381

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

Merged
merged 1 commit into from
Jul 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
28 changes: 25 additions & 3 deletions cargo-nextest/src/dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use nextest_filtering::FilteringExpr;
use nextest_metadata::{BinaryListSummary, BuildPlatform};
use nextest_runner::{
cargo_config::{CargoConfigs, TargetTriple},
config::{NextestConfig, NextestProfile, TestThreads},
config::{NextestConfig, NextestProfile, TestThreads, ToolConfigFile},
errors::WriteTestListError,
list::{BinaryList, OutputFormat, RustTestArtifact, SerializableFormat, TestList},
partition::PartitionerBuilder,
Expand Down Expand Up @@ -164,10 +164,27 @@ impl AppOpts {
}

#[derive(Debug, Args)]
#[clap(next_help_heading = "CONFIG OPTIONS")]
struct ConfigOpts {
/// Config file [default: workspace-root/.config/nextest.toml]
#[clap(long, global = true, value_name = "PATH")]
pub config_file: Option<Utf8PathBuf>,

/// Tool-specific config files
///
/// Some tools on top of nextest may want to set up their own default configuration but
/// prioritize user configuration on top. Use this argument to insert configuration
/// that's lower than --config-file in priority but above the default config shipped with
/// nextest.
///
/// Arguments are specified in the format "tool:abs_path", for example
/// "my-tool:/path/to/nextest.toml" (or "my-tool:C:\\path\\to\\nextest.toml" on Windows).
/// Paths must be absolute.
///
/// This argument may be specified multiple times. Files that come later are lower priority
/// than those that come earlier.
#[clap(long = "tool-config-file", global = true, value_name = "TOOL:ABS_PATH")]
pub tool_config_files: Vec<ToolConfigFile>,
}

impl ConfigOpts {
Expand All @@ -177,8 +194,13 @@ impl ConfigOpts {
workspace_root: &Utf8Path,
graph: &PackageGraph,
) -> Result<NextestConfig> {
NextestConfig::from_sources(workspace_root, graph, self.config_file.as_deref())
.map_err(ExpectedError::config_parse_error)
NextestConfig::from_sources(
workspace_root,
graph,
self.config_file.as_deref(),
&self.tool_config_files,
)
.map_err(ExpectedError::config_parse_error)
}
}

Expand Down
182 changes: 173 additions & 9 deletions nextest-runner/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
use crate::{
errors::{
ConfigParseError, ConfigParseErrorKind, ConfigParseOverrideError, ProfileNotFound,
TestThreadsParseError,
TestThreadsParseError, ToolConfigFileParseError,
},
reporter::{FinalStatusLevel, StatusLevel, TestOutputDisplay},
};
Expand Down Expand Up @@ -54,17 +54,29 @@ impl NextestConfig {
/// Reads the nextest config from the given file, or if not specified from `.config/nextest.toml`
/// in the workspace root.
///
/// If the file isn't specified and the directory doesn't have `.config/nextest.toml`, uses the
/// `tool_config_files` are lower priority than `config_file` but higher priority than the
/// default config. Files in `tool_config_files` that come earlier are higher priority than those
/// that come later.
///
/// If no config files are specified and this file doesn't have `.config/nextest.toml`, uses the
/// default config options.
pub fn from_sources(
pub fn from_sources<'a, I>(
workspace_root: impl Into<Utf8PathBuf>,
graph: &PackageGraph,
config_file: Option<&Utf8Path>,
) -> Result<Self, ConfigParseError> {
tool_config_files: impl IntoIterator<IntoIter = I>,
) -> Result<Self, ConfigParseError>
where
I: Iterator<Item = &'a ToolConfigFile> + DoubleEndedIterator,
{
let workspace_root = workspace_root.into();
let (config_file, config) = Self::read_from_sources(&workspace_root, config_file)?;
let tool_config_files_rev = tool_config_files.into_iter().rev();
let (config_file, config) =
Self::read_from_sources(&workspace_root, config_file, tool_config_files_rev)?;
let inner: NextestConfigImpl =
serde_path_to_error::deserialize(config).map_err(|error| {
// TODO: now that lowpri configs exist, we need better attribution for the exact path at which
// an error occurred.
ConfigParseError::new(
config_file.clone(),
ConfigParseErrorKind::DeserializeError(error),
Expand Down Expand Up @@ -109,12 +121,22 @@ impl NextestConfig {
// Helper methods
// ---

fn read_from_sources(
fn read_from_sources<'a>(
workspace_root: &Utf8Path,
file: Option<&Utf8Path>,
tool_config_files_rev: impl Iterator<Item = &'a ToolConfigFile>,
) -> Result<(Utf8PathBuf, Config), ConfigParseError> {
// First, get the default config.
let builder = Self::make_default_config();
let mut builder = Self::make_default_config();

// Next, merge in tool configs.
for ToolConfigFile {
config_file,
tool: _,
} in tool_config_files_rev
{
builder = builder.add_source(File::new(config_file.as_str(), FileFormat::Toml));
}

// Next, merge in the config from the given file.
let (builder, config_path) = match file {
Expand Down Expand Up @@ -169,6 +191,54 @@ impl NextestConfig {
}
}

/// A tool-specific config file.
///
/// Tool-specific config files are lower priority than repository configs, but higher priority than
/// the default config shipped with nextest.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct ToolConfigFile {
/// The name of the tool.
pub tool: String,

/// The path to the config file.
pub config_file: Utf8PathBuf,
}

impl FromStr for ToolConfigFile {
type Err = ToolConfigFileParseError;

fn from_str(input: &str) -> Result<Self, Self::Err> {
match input.split_once(':') {
Some((tool, config_file)) => {
if tool.is_empty() {
Err(ToolConfigFileParseError::EmptyToolName {
input: input.to_owned(),
})
} else if config_file.is_empty() {
Err(ToolConfigFileParseError::EmptyConfigFile {
input: input.to_owned(),
})
} else {
let config_file = Utf8Path::new(config_file);
if config_file.is_absolute() {
Ok(Self {
tool: tool.to_owned(),
config_file: Utf8PathBuf::from(config_file),
})
} else {
Err(ToolConfigFileParseError::ConfigFileNotAbsolute {
config_file: config_file.to_owned(),
})
}
}
}
None => Err(ToolConfigFileParseError::InvalidFormat {
input: input.to_owned(),
}),
}
}
}

/// A configuration profile for nextest. Contains most configuration used by the nextest runner.
///
/// Returned by [`NextestConfig::profile`].
Expand Down Expand Up @@ -790,7 +860,7 @@ mod tests {
let graph = temp_workspace(workspace_path, config_contents);

let nextest_config_result =
NextestConfig::from_sources(graph.workspace().root(), &graph, None);
NextestConfig::from_sources(graph.workspace().root(), &graph, None, []);

match expected_default {
Ok(expected_default) => {
Expand Down Expand Up @@ -918,7 +988,8 @@ mod tests {
let graph = temp_workspace(workspace_path, config_contents);
let package_id = graph.workspace().iter().next().unwrap().id();

let config = NextestConfig::from_sources(graph.workspace().root(), &graph, None).unwrap();
let config =
NextestConfig::from_sources(graph.workspace().root(), &graph, None, []).unwrap();
let query = TestQuery {
binary_query: BinaryQuery {
package_id,
Expand All @@ -939,6 +1010,99 @@ mod tests {
);
}

#[test]
fn parse_tool_config_file() {
cfg_if::cfg_if! {
if #[cfg(windows)] {
let valid = ["tool:C:\\foo\\bar", "tool:\\\\?\\C:\\foo\\bar"];
let invalid = ["C:\\foo\\bar", "tool:\\foo\\bar", "tool:", ":/foo/bar"];
} else {
let valid = ["tool:/foo/bar"];
let invalid = ["/foo/bar", "tool:", ":/foo/bar", "tool:foo/bar"];
}
}

for valid_input in valid {
valid_input.parse::<ToolConfigFile>().unwrap_or_else(|err| {
panic!("valid input {valid_input} should parse correctly: {err}")
});
}

for invalid_input in invalid {
invalid_input
.parse::<ToolConfigFile>()
.expect_err(&format!("invalid input {invalid_input} should error out"));
}
}

#[test]
fn lowpri_config() {
let config_contents = r#"
[profile.default]
retries = 3
"#;

let lowpri1_config_contents = r#"
[profile.default]
retries = 4

[profile.lowpri]
retries = 12
"#;

let lowpri2_config_contents = r#"
[profile.default]
retries = 5

[profile.lowpri]
retries = 16

[profile.lowpri2]
retries = 18
"#;

let workspace_dir = tempdir().unwrap();
let workspace_path: &Utf8Path = workspace_dir.path().try_into().unwrap();

let graph = temp_workspace(workspace_path, config_contents);
let workspace_root = graph.workspace().root();
let lowpri1_path = workspace_root.join(".config/lowpri1.toml");
let lowpri2_path = workspace_root.join(".config/lowpri2.toml");
std::fs::write(&lowpri1_path, lowpri1_config_contents).unwrap();
std::fs::write(&lowpri2_path, lowpri2_config_contents).unwrap();

let config = NextestConfig::from_sources(
workspace_root,
&graph,
None,
&[
ToolConfigFile {
tool: "lowpri1".to_owned(),
config_file: lowpri1_path,
},
ToolConfigFile {
tool: "lowpri2".to_owned(),
config_file: lowpri2_path,
},
],
)
.expect("parsing config failed");

let default_profile = config
.profile(NextestConfig::DEFAULT_PROFILE)
.expect("default profile is present");
// This is present in .config/nextest.toml and is the highest priority
assert_eq!(default_profile.retries(), 3);

let lowpri_profile = config.profile("lowpri").expect("lowpri profile is present");
assert_eq!(lowpri_profile.retries(), 12);

let lowpri2_profile = config
.profile("lowpri2")
.expect("lowpri2 profile is present");
assert_eq!(lowpri2_profile.retries(), 18);
}

fn temp_workspace(temp_dir: &Utf8Path, config_contents: &str) -> PackageGraph {
Command::new(cargo_path())
.args(["init", "--lib", "--name=test-package"])
Expand Down
39 changes: 37 additions & 2 deletions nextest-runner/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,13 +136,48 @@ impl StatusLevelParseError {
}
}

/// Error returned while parsing a [`ToolConfigFile`](crate::config::ToolConfigFile) value.
#[derive(Clone, Debug, Error)]
pub enum ToolConfigFileParseError {
#[error(
"tool-config-file has invalid format: {input}\n(hint: tool configs must be in the format <tool-name>:<path>)"
)]
/// The input was not in the format "tool:path".
InvalidFormat {
/// The input that failed to parse.
input: String,
},

/// The tool name was empty.
#[error("tool-config-file has empty tool name: {input}")]
EmptyToolName {
/// The input that failed to parse.
input: String,
},

/// The config file path was empty.
#[error("tool-config-file has empty config file path: {input}")]
EmptyConfigFile {
/// The input that failed to parse.
input: String,
},

/// The config file was not an absolute path.
#[error("tool-config-file is not an absolute path: {config_file}")]
ConfigFileNotAbsolute {
/// The file name that wasn't absolute.
config_file: Utf8PathBuf,
},
}

/// Error returned while parsing a [`TestThreads`](crate::config::TestThreads) value.
#[derive(Clone, Debug, Error)]
#[error(
"unrecognized value for test-threads: {input}\n(expected either an integer or \"num-cpus\")"
"unrecognized value for test-threads: {input}\n(hint: expected either an integer or \"num-cpus\")"
)]
pub struct TestThreadsParseError {
input: String,
/// The input that failed to parse.
pub input: String,
}

impl TestThreadsParseError {
Expand Down
12 changes: 4 additions & 8 deletions nextest-runner/tests/integration/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,7 @@ fn test_run() -> Result<()> {

let test_filter = TestFilterBuilder::any(RunIgnored::Default);
let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty());
let config = NextestConfig::from_sources(workspace_root(), &*PACKAGE_GRAPH, None)
.expect("loaded fixture config");
let config = load_config();
let profile = config
.profile(NextestConfig::DEFAULT_PROFILE)
.expect("default config is valid");
Expand Down Expand Up @@ -180,8 +179,7 @@ fn test_run_ignored() -> Result<()> {

let test_filter = TestFilterBuilder::any(RunIgnored::IgnoredOnly);
let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty());
let config = NextestConfig::from_sources(workspace_root(), &*PACKAGE_GRAPH, None)
.expect("loaded fixture config");
let config = load_config();
let profile = config
.profile(NextestConfig::DEFAULT_PROFILE)
.expect("default config is valid");
Expand Down Expand Up @@ -368,8 +366,7 @@ fn test_retries(retries: Option<usize>) -> Result<()> {

let test_filter = TestFilterBuilder::any(RunIgnored::Default);
let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty());
let config = NextestConfig::from_sources(workspace_root(), &*PACKAGE_GRAPH, None)
.expect("loaded fixture config");
let config = load_config();
let profile = config
.profile("with-retries")
.expect("with-retries config is valid");
Expand Down Expand Up @@ -507,8 +504,7 @@ fn test_termination() -> Result<()> {
);

let test_list = FIXTURE_TARGETS.make_test_list(&test_filter, &TargetRunner::empty());
let config = NextestConfig::from_sources(workspace_root(), &*PACKAGE_GRAPH, None)
.expect("loaded fixture config");
let config = load_config();
let profile = config
.profile("with-termination")
.expect("with-termination config is valid");
Expand Down
Loading