-
Notifications
You must be signed in to change notification settings - Fork 3
scheduler: use per-group average duration for unknown tests #114
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
Changes from all commits
bc2b4c1
895798b
faf7ba6
57b21cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,8 +100,9 @@ impl Scheduler { | |
| /// | ||
| /// * `tests` - Tests to schedule | ||
| /// * `durations` - Historical test durations from previous runs. | ||
| /// Tests not in the map use `default_duration`. | ||
| /// * `default_duration` - Duration to use for tests without historical data. | ||
| /// Tests not in the map use the per-group average from `group_to_default_duration`. | ||
| /// * `group_to_default_duration` - Per-group average duration for tests without historical data. | ||
| /// Falls back to 1 second if the group has no entry. | ||
| /// * `max_batch_duration` - Optional cap on the total duration of each batch. | ||
| /// A single test that exceeds the cap is still placed alone in its own batch. | ||
| /// | ||
|
|
@@ -120,7 +121,7 @@ impl Scheduler { | |
| &self, | ||
| tests: &[TestInstance<'a>], | ||
| durations: &HashMap<String, Duration>, | ||
| default_duration: Duration, | ||
| group_to_default_duration: &HashMap<String, Duration>, | ||
| max_batch_duration: Option<Duration>, | ||
| ) -> Vec<Vec<TestInstance<'a>>> { | ||
| if tests.is_empty() { | ||
|
|
@@ -134,11 +135,17 @@ impl Scheduler { | |
| let duration = match durations.get(t.id()) { | ||
| Some(&d) => d, | ||
| None => { | ||
| let fallback = group_to_default_duration | ||
| .get(t.group()) | ||
| .copied() | ||
| .unwrap_or(Duration::from_secs(1)); | ||
| tracing::warn!( | ||
| "No historical duration for test '{}', using default", | ||
| t.id() | ||
| "No historical duration for test '{}', using group '{}' default {:?}", | ||
| t.id(), | ||
| t.group(), | ||
| fallback, | ||
| ); | ||
| default_duration | ||
| fallback | ||
| } | ||
| }; | ||
| (*t, duration) | ||
|
|
@@ -192,7 +199,7 @@ mod tests { | |
| let scheduler = Scheduler::new(4); | ||
| let durations = HashMap::new(); | ||
| let batches: Vec<Vec<TestInstance>> = | ||
| scheduler.schedule(&[], &durations, Duration::from_secs(1), None); | ||
| scheduler.schedule(&[], &durations, &HashMap::new(), None); | ||
| assert!(batches.is_empty()); | ||
| } | ||
|
|
||
|
|
@@ -211,7 +218,7 @@ mod tests { | |
| durations.insert("medium_test".to_string(), Duration::from_secs(5)); | ||
| durations.insert("fast_test".to_string(), Duration::from_secs(1)); | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [test_coverage] (severity 3/5) (confidence 0.92) No test was added to verify the per-group average duration behavior. There should be a test where some tests in a group have historical durations and others don't, verifying that the unknown tests use the group average rather than the 1s fallback. The existing test |
||
| let batches = scheduler.schedule(&tests, &durations, Duration::from_secs(1), None); | ||
| let batches = scheduler.schedule(&tests, &durations, &HashMap::new(), None); | ||
|
|
||
| // With LPT: | ||
| // 1. Assign slow_test (10s) to worker 0 -> loads: [10, 0] | ||
|
|
@@ -241,7 +248,7 @@ mod tests { | |
| durations.insert("test_b".to_string(), Duration::from_secs(5)); | ||
| durations.insert("test_c".to_string(), Duration::from_secs(3)); | ||
|
|
||
| let batches = scheduler.schedule(&tests, &durations, Duration::from_secs(1), None); | ||
| let batches = scheduler.schedule(&tests, &durations, &HashMap::new(), None); | ||
|
|
||
| // Each test in its own batch (3 workers, 3 tests) | ||
| // Sorted by duration: test_b (5s), test_c (3s), test_a (1s) | ||
|
|
@@ -264,7 +271,7 @@ mod tests { | |
| durations.insert("known_slow".to_string(), Duration::from_secs(10)); | ||
| // unknown_test will use default of 1 second | ||
|
|
||
| let batches = scheduler.schedule(&tests, &durations, Duration::from_secs(1), None); | ||
| let batches = scheduler.schedule(&tests, &durations, &HashMap::new(), None); | ||
|
|
||
| assert_eq!(batches.len(), 2); | ||
| // known_slow (10s) should be in heaviest batch | ||
|
|
@@ -286,7 +293,7 @@ mod tests { | |
| let mut durations = HashMap::new(); | ||
| durations.insert("test_a".to_string(), Duration::from_secs(5)); | ||
|
|
||
| let batches = scheduler.schedule(&tests, &durations, Duration::from_secs(1), None); | ||
| let batches = scheduler.schedule(&tests, &durations, &HashMap::new(), None); | ||
|
|
||
| // Each instance of test_a must be in a different batch | ||
| assert_eq!(batches.len(), 3); | ||
|
|
@@ -319,7 +326,7 @@ mod tests { | |
| durations.insert("test_b".to_string(), Duration::from_secs(5)); | ||
| durations.insert("test_c".to_string(), Duration::from_secs(1)); | ||
|
|
||
| let batches = scheduler.schedule(&tests, &durations, Duration::from_secs(1), None); | ||
| let batches = scheduler.schedule(&tests, &durations, &HashMap::new(), None); | ||
|
|
||
| // Verify no batch contains duplicate test IDs | ||
| for batch in &batches { | ||
|
|
@@ -348,7 +355,7 @@ mod tests { | |
| let tests: Vec<_> = records.iter().map(|r| r.test()).collect(); | ||
|
|
||
| let durations = HashMap::new(); | ||
| let batches = scheduler.schedule(&tests, &durations, Duration::from_secs(1), None); | ||
| let batches = scheduler.schedule(&tests, &durations, &HashMap::new(), None); | ||
|
|
||
| // Each instance must be in a separate batch | ||
| assert_eq!(batches.len(), 3); | ||
|
|
@@ -379,7 +386,7 @@ mod tests { | |
| let batches = scheduler.schedule( | ||
| &tests, | ||
| &durations, | ||
| Duration::from_secs(1), | ||
| &HashMap::new(), | ||
| Some(MAX_BATCH_DURATION), | ||
| ); | ||
|
|
||
|
|
@@ -418,7 +425,7 @@ mod tests { | |
| let batches = scheduler.schedule( | ||
| &tests, | ||
| &durations, | ||
| Duration::from_secs(1), | ||
| &HashMap::new(), | ||
| Some(MAX_BATCH_DURATION), | ||
| ); | ||
|
|
||
|
|
@@ -453,7 +460,7 @@ mod tests { | |
| let batches = scheduler.schedule( | ||
| &tests, | ||
| &durations, | ||
| Duration::from_secs(1), | ||
| &HashMap::new(), | ||
| Some(MAX_BATCH_DURATION), | ||
| ); | ||
|
|
||
|
|
@@ -489,7 +496,7 @@ mod tests { | |
| ]; | ||
| let tests: Vec<_> = records.iter().map(|r| r.test()).collect(); | ||
|
|
||
| let batches = scheduler.schedule(&tests, &HashMap::new(), Duration::from_secs(1), None); | ||
| let batches = scheduler.schedule(&tests, &HashMap::new(), &HashMap::new(), None); | ||
|
|
||
| // Two tests that each use >half the command length budget must be in separate batches | ||
| assert_eq!(batches.len(), 2); | ||
|
|
@@ -506,7 +513,7 @@ mod tests { | |
| .collect(); | ||
| let tests: Vec<_> = records.iter().map(|r| r.test()).collect(); | ||
|
|
||
| let batches = scheduler.schedule(&tests, &HashMap::new(), Duration::from_secs(0), None); | ||
| let batches = scheduler.schedule(&tests, &HashMap::new(), &HashMap::new(), None); | ||
|
|
||
| // Total command length is ~400 chars, well under 30k — should be 1 batch | ||
| assert_eq!(batches.len(), 1); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[test_coverage] (severity 3/5) (confidence 0.92)
The diff adds per-group average duration logic but no test verifies this behavior. All existing scheduler tests pass empty
HashMap::new()forgroup_defaults, so the per-group fallback path (where a group has a computed average that differs from the 1s default) is never tested. A test should be added where some tests have historical durations and others in the same group don't, verifying the group average is used instead of the 1s fallback.