Skip to content

Commit

Permalink
[nextest-runner] add a --tool-config-file option
Browse files Browse the repository at this point in the history
Add an option for tools that integrate with nextest to specify configs
that are lower than .config/nextest.toml in priority, but higher than
the default config.
  • Loading branch information
sunshowers committed Jul 22, 2022
1 parent 88bc403 commit a77c393
Show file tree
Hide file tree
Showing 6 changed files with 246 additions and 24 deletions.
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:/foo/bar", "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

0 comments on commit a77c393

Please sign in to comment.