add tests for checking appraisal outputs are ordered by investment priority correctly #1091

Aurashk · 2026-01-20T14:08:31Z

Description

This PR will add a suite of tests which check that a vector of appraisal outputs are appraised in the correct order. I.e the best asset we expect will be selected.

Fixes #1011

Type of change

Bug fix (non-breaking change to fix an issue)
New feature (non-breaking change to add functionality)
Refactoring (non-breaking, non-functional change to improve maintainability)
Optimization (non-breaking change to speed up the code)
Breaking change (whatever its nature)
Documentation (improve or add documentation)

Key checklist

[ X] All tests pass: $ cargo test
[ X] The documentation builds and looks OK: $ cargo doc
Update release notes for the latest release if this PR adds a new feature or fixes a bug
present in the previous release

Further checks

[ X] Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

codecov · 2026-01-20T14:09:48Z

Codecov Report

❌ Patch coverage is 98.57143% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.29%. Comparing base (195c94c) to head (e7ff372).

Files with missing lines	Patch %	Lines
src/simulation/investment/appraisal.rs	98.48%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1091      +/-   ##
==========================================
+ Coverage   85.14%   85.29%   +0.15%     
==========================================
  Files          55       55              
  Lines        7559     7610      +51     
  Branches     7559     7610      +51     
==========================================
+ Hits         6436     6491      +55     
+ Misses        816      812       -4     
  Partials      307      307

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Aurashk · 2026-01-20T14:15:40Z

@alexdewar @tsmbland
When you get a chance please see if you're happy with how this is structured. It should be flexible enough to give us good coverage over the different types of appraisaloutputs. I've written one simple test for now. A couple of decisions I'm not too sure about though:

does it make sense to have appraisal_outputs(..) in fixtures.rs? I'm not sure if it will be used in other places besides these tests.
Since investment.rs is already quite big and complicated I'm wondering if we should move sort_appraisal_outputs_by_investment_priority to appraisal.rs instead (and put the tests there). You would have to move some other stuff along with it/expose more stuff to appraisal.rs. But might be more manageable.

tsmbland

Seems reasonable to me!

does it make sense to have appraisal_outputs(..) in fixtures.rs? I'm not sure if it will be used in other places besides these tests.

Personally, I think probably not

Since investment.rs is already quite big and complicated I'm wondering if we should move sort_appraisal_outputs_by_investment_priority to appraisal.rs instead (and put the tests there). You would have to move some other stuff along with it/expose more stuff to appraisal.rs. But might be more manageable.

Yeah that seems reasonable to me

src/fixture.rs

simplified appraisal_outputs fixture

Copilot

Pull request overview

This PR adds comprehensive test coverage for appraisal output sorting logic, addressing issue #1011. The changes extract the sorting logic into a new public function sort_appraisal_outputs_by_investment_priority to make it more testable and reusable, while adding a suite of tests to verify correct ordering behavior.

Changes:

Extracted appraisal output sorting logic into a new public function with zero-capacity filtering
Moved compare_asset_fallback function and its test from investment module to appraisal module
Added 7 new test cases covering LCOX/NPV metric sorting, tie-breaking behavior, and edge cases
Added Default derive to ObjectiveCoefficients to support test fixtures

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/simulation/investment/appraisal/coefficients.rs	Added `Default` derive to `ObjectiveCoefficients` struct to enable creating default instances in test fixtures
src/simulation/investment/appraisal.rs	Added `sort_appraisal_outputs_by_investment_priority` function, moved `compare_asset_fallback` function, added test fixture `appraisal_outputs`, and added 7 comprehensive test cases for sorting behavior
src/simulation/investment.rs	Refactored to use the new public sorting function, removed duplicated `compare_asset_fallback` function and its test (moved to appraisal module)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/simulation/investment/appraisal.rs

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T11:25:14Z

src/simulation/investment/appraisal.rs

+    /// Test that when metrics are equal, commissioned assets are sorted by commission year (newer first)
+    #[rstest]
+    fn appraisal_sort_by_commission_year_when_metrics_equal(
+        process: Process,
+        region_id: RegionID,
+        agent_id: AgentID,
+    ) {
+        let process_rc = Rc::new(process);
+        let capacity = Capacity(10.0);
+        let commission_years = [2015, 2020, 2010];
+
+        let assets: Vec<_> = commission_years
+            .iter()
+            .map(|&year| {
+                Asset::new_commissioned(
+                    agent_id.clone(),
+                    process_rc.clone(),
+                    region_id.clone(),
+                    capacity,
+                    year,
+                )
+                .unwrap()
+            })
+            .collect();
+
+        // All metrics have the same value
+        let metrics: Vec<Box<dyn MetricTrait>> = vec![
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+        ];
+
+        let outputs = appraisal_outputs(
+            assets,
+            metrics,
+            Asset::new_commissioned(agent_id, process_rc, region_id, capacity, 2015).unwrap(),
+        );
+        let sorted = sort_appraisal_outputs_by_investment_priority(outputs);
+
+        // Should be sorted by commission year, newest first: 2020, 2015, 2010
+        assert_eq!(sorted[0].asset.commission_year(), 2020);
+        assert_eq!(sorted[1].asset.commission_year(), 2015);
+        assert_eq!(sorted[2].asset.commission_year(), 2010);
+    }


Consider adding an integration test that verifies the sorting behavior when commissioned and candidate assets have equal metrics. The existing test at line 551 (compare_assets_fallback) tests the comparison function directly, but there's no test that verifies the full sorting pipeline correctly prioritizes commissioned assets over candidate assets when metrics are equal. This would ensure the tie-breaking logic works correctly in the actual sorting context.

I'm not sure what you mean here copilot, the test you are describing sounds exactly like one we added in this PR.

Do we compare a commissioned with a non-commissioned asset in any of the tests though?

alexdewar

Thanks for doing this! I've suggested a few small tweaks, but otherwise good to go.

alexdewar · 2026-01-22T09:21:09Z

src/simulation/investment/appraisal.rs

+            Ordering::Equal => compare_asset_fallback(&output1.asset, &output2.asset),
+            cmp => cmp,
+        })
+        .collect_vec()


As it is, this function could take its argument as a reference, because the existing Vec is copied, but it'd be better to avoid that.

You could just modify the Vec in place:

outputs_for_ops.retain(|output| output.capacity.total_capacity() > Capacity(0.0)); outputs_for_opts.sort_by(|output1, output2| match output1.compare_metric(output2) { // If equal, we fall back on comparing asset properties Ordering::Equal => compare_asset_fallback(&output1.asset, &output2.asset), cmp => cmp, });

I'd personally make the argument a mut ref rather than consuming it then returning it, but that's more of a stylistic thing.

alexdewar · 2026-01-22T09:31:14Z

src/simulation/investment/appraisal/coefficients.rs

 /// the investment appraisal routines. The map contains the per-capacity and per-activity cost
 /// coefficients used in the appraisal optimisation, together with the unmet-demand penalty.
-#[derive(Clone)]
+#[derive(Clone, Default)]


I'm not sure it really makes sense to have a Default implementation for this. I know you're just using it for tests where it doesn't matter. I'd either define a fixture for it or you can use conditional compilation:

Suggested change

#[derive(Clone, Default)]

#[derive(Clone)]

#[cfg_attr(test, derive(Default))]

alexdewar · 2026-01-22T10:14:57Z

src/simulation/investment/appraisal.rs

+    #[fixture]
+    fn appraisal_outputs(
+        #[default(vec![])] assets: Vec<Asset>,
+        #[default(vec![])] metrics: Vec<Box<dyn MetricTrait>>,
+        asset: Asset,
+    ) -> Vec<AppraisalOutput> {


Given that you don't use it as a fixture (as in, passed into tests via input args), I don't think you need to make it one:

Suggested change

#[fixture]

fn appraisal_outputs(

#[default(vec![])] assets: Vec<Asset>,

#[default(vec![])] metrics: Vec<Box<dyn MetricTrait>>,

asset: Asset,

) -> Vec<AppraisalOutput> {

fn appraisal_outputs(

assets: Vec<Asset>,

metrics: Vec<Box<dyn MetricTrait>>,

asset: Asset,

) -> Vec<AppraisalOutput> {

I'm also not sure about using an empty Vec as a sentinel value for assets. I think it'd be cleaner either to make it an Option<Vec<Asset>> or have a separate function which uses default assets.

alexdewar · 2026-01-22T10:18:26Z

src/simulation/investment/appraisal.rs

+    /// Test that when metrics are equal, commissioned assets are sorted by commission year (newer first)
+    #[rstest]
+    fn appraisal_sort_by_commission_year_when_metrics_equal(
+        process: Process,
+        region_id: RegionID,
+        agent_id: AgentID,
+    ) {
+        let process_rc = Rc::new(process);
+        let capacity = Capacity(10.0);
+        let commission_years = [2015, 2020, 2010];
+
+        let assets: Vec<_> = commission_years
+            .iter()
+            .map(|&year| {
+                Asset::new_commissioned(
+                    agent_id.clone(),
+                    process_rc.clone(),
+                    region_id.clone(),
+                    capacity,
+                    year,
+                )
+                .unwrap()
+            })
+            .collect();
+
+        // All metrics have the same value
+        let metrics: Vec<Box<dyn MetricTrait>> = vec![
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+        ];
+
+        let outputs = appraisal_outputs(
+            assets,
+            metrics,
+            Asset::new_commissioned(agent_id, process_rc, region_id, capacity, 2015).unwrap(),
+        );
+        let sorted = sort_appraisal_outputs_by_investment_priority(outputs);
+
+        // Should be sorted by commission year, newest first: 2020, 2015, 2010
+        assert_eq!(sorted[0].asset.commission_year(), 2020);
+        assert_eq!(sorted[1].asset.commission_year(), 2015);
+        assert_eq!(sorted[2].asset.commission_year(), 2010);
+    }


Do we compare a commissioned with a non-commissioned asset in any of the tests though?

alexdewar · 2026-01-22T10:21:47Z

src/simulation/investment/appraisal.rs

+        for (i, &expected_id) in agent_ids.iter().enumerate() {
+            assert_eq!(
+                sorted[i].asset.agent_id(),
+                Some(&AgentID(expected_id.into()))
+            );


What about:

Suggested change

for (i, &expected_id) in agent_ids.iter().enumerate() {

assert_eq!(

sorted[i].asset.agent_id(),

Some(&AgentID(expected_id.into()))

);

for (&expected_id, &output) in agent_ids.iter().zip(&outputs) {

assert_eq!(

output.asset.agent_id(),

Some(&AgentID(expected_id.into()))

);

I've probably put an ampersand in the wrong place somewhere there, but you get the idea 😉

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-23T11:13:36Z

src/simulation/investment/appraisal.rs

+    /// Test that when metrics are equal, commissioned assets are sorted by commission year (newer first)
+    #[rstest]
+    fn appraisal_sort_by_commission_year_when_metrics_equal(
+        process: Process,
+        region_id: RegionID,
+        agent_id: AgentID,
+    ) {
+        let process_rc = Rc::new(process);
+        let capacity = Capacity(10.0);
+        let commission_years = [2015, 2020, 2010];
+
+        let assets: Vec<_> = commission_years
+            .iter()
+            .map(|&year| {
+                Asset::new_commissioned(
+                    agent_id.clone(),
+                    process_rc.clone(),
+                    region_id.clone(),
+                    capacity,
+                    year,
+                )
+                .unwrap()
+            })
+            .collect();
+
+        // All metrics have the same value
+        let metrics: Vec<Box<dyn MetricTrait>> = vec![
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+        ];
+
+        let mut outputs = appraisal_outputs(assets, metrics);
+        sort_appraisal_outputs_by_investment_priority(&mut outputs);
+
+        // Should be sorted by commission year, newest first: 2020, 2015, 2010
+        assert_eq!(outputs[0].asset.commission_year(), 2020);
+        assert_eq!(outputs[1].asset.commission_year(), 2015);
+        assert_eq!(outputs[2].asset.commission_year(), 2010);
+    }


Consider adding an integration test that verifies commissioned assets are prioritized over candidate assets when metrics are equal. While the compare_assets_fallback function is tested directly (line 506), there's no test that verifies the full sort_appraisal_outputs_by_investment_priority pipeline correctly handles the tie-breaking between commissioned and candidate assets. This would ensure the sorting behavior works correctly end-to-end. For example, create a test with multiple assets (some commissioned, some candidates) that all have equal metrics and verify that commissioned assets appear before candidates in the sorted result.

Copilot · 2026-01-23T11:13:37Z

src/simulation/investment/appraisal.rs

+        metrics: Vec<Box<dyn MetricTrait>>,
+        asset: &Asset,
+    ) -> Vec<AppraisalOutput> {
+        // If no assets provided, repeat the default asset for each metric.


The comment "If no assets provided" is misleading since the function requires an asset parameter. Consider updating the comment to accurately reflect the function's behavior, such as "Repeat the provided asset for each metric."

Suggested change

// If no assets provided, repeat the default asset for each metric.

// Repeat the provided asset for each metric.

Aurashk · 2026-01-23T11:27:34Z

Thanks @alexdewar, all good suggestions. I added a couple more test cases too which were missing coverage

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/simulation/investment/appraisal.rs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-23T11:34:22Z

src/simulation/investment/appraisal.rs

+    fn appraisal_sort_maintains_order_when_all_equal(process: Process, region_id: RegionID) {
+        let process_rc = Rc::new(process);
+        let capacity = Capacity(10.0);
+        let commission_year = 2015;
+        let agent_ids = ["agent1", "agent2", "agent3"];
+
+        let assets: Vec<_> = agent_ids
+            .iter()
+            .map(|&id| {
+                Asset::new_commissioned(
+                    AgentID(id.into()),
+                    process_rc.clone(),
+                    region_id.clone(),
+                    capacity,
+                    commission_year,
+                )
+                .unwrap()
+            })
+            .collect();
+
+        let metrics: Vec<Box<dyn MetricTrait>> = vec![
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+            Box::new(LCOXMetric::new(MoneyPerActivity(5.0))),
+        ];
+
+        let mut outputs = appraisal_outputs(assets.clone(), metrics);
+        sort_appraisal_outputs_by_investment_priority(&mut outputs);
+
+        // Verify order is preserved - should match the original agent_ids array
+        for (&expected_id, output) in agent_ids.iter().zip(outputs) {
+            assert_eq!(output.asset.agent_id(), Some(&AgentID(expected_id.into())));
+        }
+    }


The test expects that order is preserved when all metrics and commission years are equal, but sort_by is not a stable sort in Rust - it doesn't guarantee preservation of original order for equal elements. This test may pass or fail depending on implementation details and could lead to flaky tests.

Consider either:

Using sort_by_key with a stable sort if order preservation is important for equal elements

Updating the test to not rely on order preservation when all comparison criteria are equal (since the documentation at line 343 already states "The function does not guarantee that all ties will be resolved")

Making this test less strict by only checking that the same elements are present without checking their order

I did a quick search which suggests sort_by is stable so I don't think this test is implementation dependent and should always preserve equal ordering

In Rust, sort_by performs a stable sort, meaning it preserves the relative order of elements that compare as equal. This is the default behavior for sort and sort_by methods on slices and vectors. Key Points: Stability: When using sort_by, if two elements are equal according to the comparison function, their original order in the slice is maintained. Usage: sort_by takes a custom comparison function that returns an Ordering (Less, Equal, or Greater). Example: let mut vec = vec!["banana", "apple", "cherry"]; vec.sort_by(|a, b| a.cmp(b)); // Result: ["apple", "banana", "cherry"] Stable vs Unstable: If stability is not required, sort_unstable_by can be faster and uses less memory, as it does not guarantee order preservation for equal elements.

Yeah... this seems like a hallucination

alexdewar

This is great! Good work.

You probably could have got away with just having tests for LCOX, as you're interested in testing the sorting logic here, not how individual metrics implement the comparison, but this is good and thorough.

alexdewar · 2026-01-23T12:15:08Z

src/simulation/investment/appraisal.rs

+
+    /// Creates appraisal from corresponding assets and metrics
+    ///
+    /// # Panics


Missing blank line:

Suggested change

/// # Panics

/// # Panics

///

add simple lcox metric test

a20bc5d

tsmbland reviewed Jan 20, 2026

View reviewed changes

src/fixture.rs Outdated Show resolved Hide resolved

src/fixture.rs Outdated Show resolved Hide resolved

move some appraisal logic and tests to appraisal.rs from investment.rs

2734d1f

simplified appraisal_outputs fixture

Aurashk changed the title ~~add simple lcox metric test~~ add tests for checking appraisal outputs are ordered by investment priority correctly Jan 21, 2026

Aurashk requested a review from alexdewar January 21, 2026 10:27

add appraisal output ordering tests

9976bd4

Aurashk marked this pull request as ready for review January 21, 2026 11:05

Copilot AI review requested due to automatic review settings January 21, 2026 11:05

Copilot started reviewing on behalf of Aurashk January 21, 2026 11:06 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

src/simulation/investment/appraisal.rs Outdated Show resolved Hide resolved

src/simulation/investment/appraisal.rs Outdated Show resolved Hide resolved

src/simulation/investment/appraisal.rs Outdated Show resolved Hide resolved

Aurashk added 2 commits January 21, 2026 11:13

rename variable for clarity

701b708

add a test for zero capacity sorting of appraisal outputs

a0b9e66

Copilot AI review requested due to automatic review settings January 21, 2026 11:21

Copilot started reviewing on behalf of Aurashk January 21, 2026 11:21 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

improve doc wording

80bca50

alexdewar requested changes Jan 22, 2026

View reviewed changes

Merge branch 'main' into add-appraisal-output-tests

4fde2bc

Copilot AI review requested due to automatic review settings January 23, 2026 09:40

Copilot started reviewing on behalf of Aurashk January 23, 2026 09:40 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

Aurashk added 2 commits January 23, 2026 09:51

swap appraisal output copy for mut ref

bcf5afa

make appraisal output tests cleaner

7f06bda

Copilot AI review requested due to automatic review settings January 23, 2026 11:09

Copilot started reviewing on behalf of Aurashk January 23, 2026 11:09 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

Aurashk added 2 commits January 23, 2026 11:24

add more test coverage of sort_appraisal_outputs_by_investment_priority

5144c77

remove misleading comment

f889222

Aurashk requested review from alexdewar and Copilot January 23, 2026 11:25

Copilot started reviewing on behalf of Aurashk January 23, 2026 11:26 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

src/simulation/investment/appraisal.rs Outdated Show resolved Hide resolved

Update src/simulation/investment/appraisal.rs

e7ff372

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings January 23, 2026 11:30

Copilot started reviewing on behalf of Aurashk January 23, 2026 11:30 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

alexdewar approved these changes Jan 23, 2026

View reviewed changes

	#[derive(Clone, Default)]
	#[derive(Clone)]
	#[cfg_attr(test, derive(Default))]

	// If no assets provided, repeat the default asset for each metric.
	// Repeat the provided asset for each metric.

add tests for checking appraisal outputs are ordered by investment priority correctly #1091

Are you sure you want to change the base?

add tests for checking appraisal outputs are ordered by investment priority correctly #1091

Uh oh!

Conversation

Aurashk commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Key checklist

Further checks

Uh oh!

codecov bot commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Aurashk commented Jan 20, 2026

Uh oh!

tsmbland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexdewar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Aurashk commented Jan 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Aurashk commented Jan 20, 2026 •

edited

Loading

codecov bot commented Jan 20, 2026 •

edited

Loading

Aurashk Jan 23, 2026 •

edited

Loading