Skip to content

Commit

Permalink
Revert "[nextest-runner] make count-based partitioning simpler"
Browse files Browse the repository at this point in the history 
This reverts commit 14d7791. As
@Guiguiprim pointed out in #159, we want the partition operator to apply
after others.
  • Loading branch information
@sunshowers
sunshowers committed Apr 18, 2022
1 parent 14d7791 commit 95e2f29
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 61 deletions.
35 changes: 9 additions & 26 deletions nextest-runner/src/list/test_list.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
helpers::{dylib_path, dylib_path_envvar, write_test_name},
list::{BinaryList, BinaryListState, OutputFormat, RustBuildMeta, Styles, TestListState},
target_runner::{PlatformRunner, TargetRunner},
test_filter::{FilterInstance, TestFilterBuilder},
test_filter::TestFilterBuilder,
};
use camino::{Utf8Path, Utf8PathBuf};
use duct::Expression;
Expand Down Expand Up @@ -421,7 +421,7 @@ impl<'g> TestList<'g> {
}

fn process_output(
artifact: RustTestArtifact<'g>,
test_binary: RustTestArtifact<'g>,
filter: &TestFilterBuilder,
non_ignored: impl AsRef<str>,
ignored: impl AsRef<str>,
Expand All @@ -430,45 +430,28 @@ impl<'g> TestList<'g> {

// Treat ignored and non-ignored as separate sets of single filters, so that partitioning
// based on one doesn't affect the other.
//
// TODO: now that the filter doesn't have mutable state, the indirection provided by
// TestFilterBuilder isn't required, nor is having a separate filter for non-ignored and
// ignored -- clean this up.
let non_ignored_filter = filter.build();
for (index, test_name) in Self::parse(non_ignored.as_ref())?.into_iter().enumerate() {
let instance = FilterInstance {
artifact: &artifact,
test_name,
ignored: false,
index,
};
let mut non_ignored_filter = filter.build();
for test_name in Self::parse(non_ignored.as_ref())? {
tests.insert(
test_name.into(),
RustTestCaseSummary {
ignored: false,
filter_match: non_ignored_filter.filter_match(instance),
filter_match: non_ignored_filter.filter_match(&test_binary, test_name, false),
},
);
}

let ignored_filter = filter.build();
for (index, test_name) in Self::parse(ignored.as_ref())?.into_iter().enumerate() {
let mut ignored_filter = filter.build();
for test_name in Self::parse(ignored.as_ref())? {
// Note that libtest prints out:
// * just ignored tests if --ignored is passed in
// * all tests, both ignored and non-ignored, if --ignored is not passed in
// Adding ignored tests after non-ignored ones makes everything resolve correctly.

let instance = FilterInstance {
artifact: &artifact,
test_name,
ignored: true,
index,
};
tests.insert(
test_name.into(),
RustTestCaseSummary {
ignored: true,
filter_match: ignored_filter.filter_match(instance),
filter_match: ignored_filter.filter_match(&test_binary, test_name, true),
},
);
}
Expand All @@ -480,7 +463,7 @@ impl<'g> TestList<'g> {
binary_name,
cwd,
build_platform: platform,
} = artifact;
} = test_binary;

Ok((
binary_path,
Expand Down
12 changes: 8 additions & 4 deletions nextest-runner/src/partition.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub enum PartitionerBuilder {
/// Represents an individual partitioner, typically scoped to a test binary.
pub trait Partitioner: fmt::Debug {
/// Returns true if the given test name matches the partition.
fn test_matches(&self, test_name: &str, index: usize) -> bool;
fn test_matches(&mut self, test_name: &str) -> bool;
}

impl PartitionerBuilder {
Expand Down Expand Up @@ -145,6 +145,7 @@ fn parse_shards(
struct CountPartitioner {
shard_minus_one: u64,
total_shards: u64,
curr: u64,
}

impl CountPartitioner {
Expand All @@ -153,13 +154,16 @@ impl CountPartitioner {
Self {
shard_minus_one,
total_shards,
curr: 0,
}
}
}

impl Partitioner for CountPartitioner {
fn test_matches(&self, _test_name: &str, index: usize) -> bool {
(index as u64) % self.total_shards == self.shard_minus_one
fn test_matches(&mut self, _test_name: &str) -> bool {
let matches = self.curr == self.shard_minus_one;
self.curr = (self.curr + 1) % self.total_shards;
matches
}
}

Expand All @@ -180,7 +184,7 @@ impl HashPartitioner {
}

impl Partitioner for HashPartitioner {
fn test_matches(&self, test_name: &str, _index: usize) -> bool {
fn test_matches(&mut self, test_name: &str) -> bool {
let mut hasher = XxHash64::default();
test_name.hash(&mut hasher);
hasher.finish() % self.total_shards == self.shard_minus_one
Expand Down
53 changes: 22 additions & 31 deletions nextest-runner/src/test_filter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,6 @@ impl TestFilterBuilder {
/// If an empty slice is passed, the test filter matches all possible test names.
pub fn new(
run_ignored: RunIgnored,
// TODO: PartitionerBuilder was required back when the partitioner had mutable state.
// It is no longer required so this can be simplified.
partitioner_builder: Option<PartitionerBuilder>,
patterns: &[impl AsRef<[u8]>],
exprs: Vec<FilteringExpr>,
Expand Down Expand Up @@ -137,22 +135,6 @@ impl TestFilterBuilder {
}
}

/// Instance of a test to which a filter is being applied.
#[derive(Copy, Clone, Debug)]
pub struct FilterInstance<'a> {
/// The test binary artifact.
pub artifact: &'a RustTestArtifact<'a>,

/// The name of the test.
pub test_name: &'a str,

/// Whether this test is ignored.
pub ignored: bool,

/// An increasing numeric index for this test, used for partitioning.
pub index: usize,
}

/// Test filter, scoped to a single binary.
#[derive(Debug)]
pub struct TestFilter<'builder> {
Expand All @@ -162,25 +144,30 @@ pub struct TestFilter<'builder> {

impl<'filter> TestFilter<'filter> {
/// Returns an enum describing the match status of this filter.
pub fn filter_match(&self, instance: FilterInstance<'_>) -> FilterMatch {
self.filter_ignored_mismatch(instance)
.or_else(|| self.filter_name_mismatch(instance.test_name))
.or_else(|| self.filter_expression_mismatch(instance))
.or_else(|| self.filter_partition_mismatch(instance))
pub fn filter_match(
&mut self,
test_binary: &RustTestArtifact<'_>,
test_name: &str,
ignored: bool,
) -> FilterMatch {
self.filter_ignored_mismatch(ignored)
.or_else(|| self.filter_name_mismatch(test_name))
.or_else(|| self.filter_expression_mismatch(test_binary, test_name))
.or_else(|| self.filter_partition_mismatch(test_name))
.unwrap_or(FilterMatch::Matches)
}

fn filter_ignored_mismatch(&self, instance: FilterInstance<'_>) -> Option<FilterMatch> {
fn filter_ignored_mismatch(&self, ignored: bool) -> Option<FilterMatch> {
match self.builder.run_ignored {
RunIgnored::IgnoredOnly => {
if !instance.ignored {
if !ignored {
return Some(FilterMatch::Mismatch {
reason: MismatchReason::Ignored,
});
}
}
RunIgnored::Default => {
if instance.ignored {
if ignored {
return Some(FilterMatch::Mismatch {
reason: MismatchReason::Ignored,
});
Expand All @@ -205,13 +192,17 @@ impl<'filter> TestFilter<'filter> {
}
}

fn filter_expression_mismatch(&self, instance: FilterInstance<'_>) -> Option<FilterMatch> {
fn filter_expression_mismatch(
&self,
test_binary: &RustTestArtifact<'_>,
test_name: &str,
) -> Option<FilterMatch> {
let accepted = self.builder.exprs.is_empty()
|| self
.builder
.exprs
.iter()
.any(|expr| expr.includes(instance.artifact.package.id(), instance.test_name));
.any(|expr| expr.includes(test_binary.package.id(), test_name));

match accepted {
false => Some(FilterMatch::Mismatch {
Expand All @@ -221,9 +212,9 @@ impl<'filter> TestFilter<'filter> {
}
}

fn filter_partition_mismatch(&self, instance: FilterInstance<'_>) -> Option<FilterMatch> {
let partition_match = match &self.partitioner {
Some(partitioner) => partitioner.test_matches(instance.test_name, instance.index),
fn filter_partition_mismatch(&mut self, test_name: &str) -> Option<FilterMatch> {
let partition_match = match &mut self.partitioner {
Some(partitioner) => partitioner.test_matches(test_name),
None => true,
};
if partition_match {
Expand Down

0 comments on commit 95e2f29

Please sign in to comment.