From d9d4ecd7aeb5c64dc4311092c9f6926be1a8fa9d Mon Sep 17 00:00:00 2001 From: MatthewMckee4 Date: Tue, 14 Apr 2026 14:10:09 +0100 Subject: [PATCH] Reduce cloning in `TestVariantIterator` Rework `TestVariantIterator` so producing a variant is cheap. The iterator now borrows `&DiscoveredTestFunction` from the surrounding module instead of rebuilding it into an `Rc`, and shares fixture lists across variants via `Rc<[Rc]>` so each variant costs a handful of refcount bumps rather than a full `Vec` clone per fixture set. `param_args` is consumed as an `IntoIter`, moving each `ParametrizationArgs.values` (and `.tags`) into the emitted variant; this also lets `Arc::try_unwrap` in `setup_test_fixtures` succeed, skipping a Python refcount bump per parametrize argument. Implements `Iterator` on `TestVariantIterator` so the call site in `package_runner` can use a plain `for variant in ...` loop. Also collapses a couple of `match` arms in `package_runner` into `map_err(...)?` and skips building the kwargs `PyDict` in `NormalizedFixture::call` when there are no fixture arguments. --- .../extensions/fixtures/normalized_fixture.rs | 14 +-- .../src/runner/package_runner.rs | 43 +++---- .../src/runner/test_iterator.rs | 113 +++++++++--------- 3 files changed, 82 insertions(+), 88 deletions(-) diff --git a/crates/karva_test_semantic/src/extensions/fixtures/normalized_fixture.rs b/crates/karva_test_semantic/src/extensions/fixtures/normalized_fixture.rs index e6bbc19b..f22a5be8 100644 --- a/crates/karva_test_semantic/src/extensions/fixtures/normalized_fixture.rs +++ b/crates/karva_test_semantic/src/extensions/fixtures/normalized_fixture.rs @@ -71,15 +71,15 @@ impl NormalizedFixture { py: Python, fixture_arguments: &HashMap>, ) -> PyResult> { - let kwargs_dict = PyDict::new(py); - - for (key, value) in fixture_arguments { - kwargs_dict.set_item(key, value)?; - } - - let result = if kwargs_dict.is_empty() { + let result = if fixture_arguments.is_empty() { self.py_function.call0(py) } else { + let kwargs_dict = PyDict::new(py); + + for (key, value) in fixture_arguments { + kwargs_dict.set_item(key, value)?; + } + self.py_function.call(py, (), Some(&kwargs_dict)) }; diff --git a/crates/karva_test_semantic/src/runner/package_runner.rs b/crates/karva_test_semantic/src/runner/package_runner.rs index 8c074769..c939500b 100644 --- a/crates/karva_test_semantic/src/runner/package_runner.rs +++ b/crates/karva_test_semantic/src/runner/package_runner.rs @@ -131,9 +131,7 @@ impl<'ctx, 'a> PackageRunner<'ctx, 'a> { let mut test_resolver = RuntimeFixtureResolver::new(parents, module); // Iterate over all test variants (parametrize combinations × fixture combinations). - // Uses next_with_py so each variant gets fresh function-scoped built-in fixtures. - let mut iterator = TestVariantIterator::new(py, test_function, &mut test_resolver); - while let Some(variant) = iterator.next_with_py(py) { + for variant in TestVariantIterator::new(py, test_function, &mut test_resolver) { let variant_passed = self.execute_test_variant(py, variant); self.record_outcome(variant_passed); passed &= variant_passed; @@ -399,7 +397,7 @@ impl<'ctx, 'a> PackageRunner<'ctx, 'a> { } /// Run a test variant (a specific combination of parametrize values and fixtures). - fn execute_test_variant(&self, py: Python<'_>, variant: TestVariant) -> bool { + fn execute_test_variant(&self, py: Python<'_>, variant: TestVariant<'_>) -> bool { let tags = variant.resolved_tags(); let test_module_path = variant.module_path().clone(); @@ -543,34 +541,27 @@ impl<'ctx, 'a> PackageRunner<'ctx, 'a> { } } - let fixture_call_result = match fixture.call(py, &function_arguments) { - Ok(fixture_call_result) => fixture_call_result, - Err(err) => { - return Err(FixtureCallError { + let fixture_call_result = + fixture + .call(py, &function_arguments) + .map_err(|err| FixtureCallError { fixture_name: fixture.name.function_name().to_string(), error: err, stmt_function_def: fixture.stmt_function_def.clone(), source_file: source_file(fixture.name.module_path().path()), arguments: function_arguments, dependency_chain: Vec::new(), - }); - } - }; - - let (final_result, finalizer) = - match get_value_and_finalizer(py, fixture, fixture_call_result) { - Ok((final_result, finalizer)) => (final_result, finalizer), - Err(err) => { - return Err(FixtureCallError { - fixture_name: fixture.name.function_name().to_string(), - error: err, - stmt_function_def: fixture.stmt_function_def.clone(), - source_file: source_file(fixture.name.module_path().path()), - arguments: HashMap::new(), - dependency_chain: Vec::new(), - }); - } - }; + })?; + + let (final_result, finalizer) = get_value_and_finalizer(py, fixture, fixture_call_result) + .map_err(|err| FixtureCallError { + fixture_name: fixture.name.function_name().to_string(), + error: err, + stmt_function_def: fixture.stmt_function_def.clone(), + source_file: source_file(fixture.name.module_path().path()), + arguments: HashMap::new(), + dependency_chain: Vec::new(), + })?; self.fixture_cache.insert( fixture.function_name().to_string(), diff --git a/crates/karva_test_semantic/src/runner/test_iterator.rs b/crates/karva_test_semantic/src/runner/test_iterator.rs index 09bc1df0..2c3147fa 100644 --- a/crates/karva_test_semantic/src/runner/test_iterator.rs +++ b/crates/karva_test_semantic/src/runner/test_iterator.rs @@ -16,27 +16,34 @@ use crate::runner::fixture_resolver::RuntimeFixtureResolver; /// - A specific set of parametrize values /// - Resolved fixture dependencies /// - Combined tags from the test and parameter set -pub(super) struct TestVariant { - /// Reference to the original discovered test function. - pub test: Rc, - - /// Parameter values for this variant (from @parametrize). +/// +/// The fixture lists are shared between every variant of a test via `Rc<[…]>`, +/// so producing a new variant is a handful of refcount bumps rather than a +/// full `Vec` clone per fixture set. +pub(super) struct TestVariant<'a> { + /// Reference to the original discovered test function. Borrowed from the + /// surrounding module, which outlives the iterator. + pub test: &'a DiscoveredTestFunction, + + /// Parameter values for this variant (from @parametrize). Moved out of + /// the owning `ParametrizationArgs` so that `Arc::try_unwrap` in the + /// caller can unwrap without a Python refcount bump. pub params: HashMap>>, /// Fixtures to be passed as arguments to the test function. - pub fixture_dependencies: Vec>, + pub fixture_dependencies: Rc<[Rc]>, /// Fixtures from @usefixtures (run for side effects, not passed as args). - pub use_fixture_dependencies: Vec>, + pub use_fixture_dependencies: Rc<[Rc]>, /// Auto-use fixtures that run automatically before this test. - pub auto_use_fixtures: Vec>, + pub auto_use_fixtures: Rc<[Rc]>, /// Combined tags from the test and its parameter set. pub tags: Tags, } -impl TestVariant { +impl TestVariant<'_> { /// Get the module path for diagnostics. pub(super) fn module_path(&self) -> &camino::Utf8PathBuf { self.test.name.module_path().path() @@ -46,15 +53,15 @@ impl TestVariant { pub(super) fn resolved_tags(&self) -> Tags { let mut tags = self.tags.clone(); - for dependency in &self.fixture_dependencies { + for dependency in self.fixture_dependencies.iter() { tags.extend(&dependency.resolved_tags()); } - for dependency in &self.use_fixture_dependencies { + for dependency in self.use_fixture_dependencies.iter() { tags.extend(&dependency.resolved_tags()); } - for dependency in &self.auto_use_fixtures { + for dependency in self.auto_use_fixtures.iter() { tags.extend(&dependency.resolved_tags()); } @@ -65,23 +72,26 @@ impl TestVariant { /// Iterates over all variants of a test function. /// /// Expands parametrize combinations to produce all concrete test invocations. -pub(super) struct TestVariantIterator { - test: Rc, - param_args: Vec, - fixture_dependencies: Vec>, - use_fixture_dependencies: Vec>, - auto_use_fixtures: Vec>, - - param_index: usize, +/// The iterator borrows the underlying `DiscoveredTestFunction` from the +/// module and shares fixture lists between variants via `Rc<[…]>`, so +/// producing N variants costs N refcount bumps rather than N deep clones. +pub(super) struct TestVariantIterator<'a> { + test: &'a DiscoveredTestFunction, + /// Consumed as we iterate, so `values` and `tags` on each + /// `ParametrizationArgs` are moved into the emitted variant (not cloned). + param_args: std::vec::IntoIter, + fixture_dependencies: Rc<[Rc]>, + use_fixture_dependencies: Rc<[Rc]>, + auto_use_fixtures: Rc<[Rc]>, } -impl TestVariantIterator { +impl<'a> TestVariantIterator<'a> { /// Create a new iterator for the given test function. /// /// Resolves fixtures and computes all parametrize variants. pub(super) fn new( py: Python, - test: &DiscoveredTestFunction, + test: &'a DiscoveredTestFunction, resolver: &mut RuntimeFixtureResolver, ) -> Self { let test_params = test.tags.parametrize_args(); @@ -95,7 +105,7 @@ impl TestVariantIterator { // use_fixtures are run for side effects but not passed as arguments. let function_param_names = test.stmt_function_def.required_fixtures(py); - let function_auto_use_fixtures = resolver.get_normalized_auto_use_fixtures( + let auto_use_fixtures = resolver.get_normalized_auto_use_fixtures( py, crate::extensions::fixtures::FixtureScope::Function, ); @@ -106,51 +116,44 @@ impl TestVariantIterator { let use_fixture_names = test.tags.required_fixtures_names(); let use_fixture_dependencies = resolver.resolve_use_fixtures(py, &use_fixture_names); - let param_args: Vec = if test_params.is_empty() { + let param_args = if test_params.is_empty() { vec![ParametrizationArgs::default()] } else { test_params }; Self { - test: Rc::new(DiscoveredTestFunction { - name: test.name.clone(), - stmt_function_def: Rc::clone(&test.stmt_function_def), - py_function: test.py_function.clone_ref(py), - tags: test.tags.clone(), - }), - param_args, - fixture_dependencies, - use_fixture_dependencies, - auto_use_fixtures: function_auto_use_fixtures, - param_index: 0, + test, + param_args: param_args.into_iter(), + fixture_dependencies: Rc::from(fixture_dependencies), + use_fixture_dependencies: Rc::from(use_fixture_dependencies), + auto_use_fixtures: Rc::from(auto_use_fixtures), } } } -impl TestVariantIterator { - /// Returns the next test variant for the current parametrize combination. - pub(super) fn next_with_py(&mut self, _py: Python<'_>) -> Option { - if self.param_index >= self.param_args.len() { - return None; - } +impl<'a> Iterator for TestVariantIterator<'a> { + type Item = TestVariant<'a>; - let param_args = &self.param_args[self.param_index]; + fn next(&mut self) -> Option { + let param_args = self.param_args.next()?; - let mut new_tags = self.test.tags.clone(); - new_tags.extend(¶m_args.tags); + let mut tags = self.test.tags.clone(); + tags.extend(¶m_args.tags); - let variant = TestVariant { - test: Rc::clone(&self.test), - params: param_args.values.clone(), - fixture_dependencies: self.fixture_dependencies.clone(), - use_fixture_dependencies: self.use_fixture_dependencies.clone(), - auto_use_fixtures: self.auto_use_fixtures.clone(), - tags: new_tags, - }; - - self.param_index += 1; + Some(TestVariant { + test: self.test, + params: param_args.values, + fixture_dependencies: Rc::clone(&self.fixture_dependencies), + use_fixture_dependencies: Rc::clone(&self.use_fixture_dependencies), + auto_use_fixtures: Rc::clone(&self.auto_use_fixtures), + tags, + }) + } - Some(variant) + fn size_hint(&self) -> (usize, Option) { + self.param_args.size_hint() } } + +impl ExactSizeIterator for TestVariantIterator<'_> {}