Delete problematic test

[tsmbland]

Description

validate_capacity_growth_limit_warning validates investment constraints and checks that an appropriate warning is raised in the logs. It does this by using logger.pop() to check the most recent log message after the validate function is run. Unfortunately, there's a problem here that other tests in src also emit logs at the same time, to the same logger. Usually logger.pop() will get the appropriate log message, regardless of what other tests are logging, but there's a race condition here that can occasionally cause other, unrelated, log message to be picked up. See here, if it hasn't expired yet, or the screenshot below.

I don't really have a solution for this, but I don't want problematic tests in the repo so I'm just going to delete it. It's not a particularly important test (especially as it's testing a feature that isn't actually implemented yet - growth limits!), and fortunately this is the only test like this that checks log messages. Longer term we should think of a solution for this kind of problem. It would probably work if we moved it to its own file in tests/, but it doesn't really seem worth it to be honest.

To try and make up for this, I've updated validate_constraints_valid to cover more scenarios, and added a TODO note about checking the log warning.

Fixes #1523

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

• All tests pass: $ cargo test
• 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

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.85%. Comparing base (7cd741d) to head (f456121).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1530      +/-   ##
==========================================
- Coverage   90.95%   90.85%   -0.10%     
==========================================
  Files          61       61              
  Lines        9050     9039      -11     
  Branches     9050     9039      -11     
==========================================
- Hits         8231     8212      -19     
- Misses        500      508       +8     
  Partials      319      319              

☔ View full report in Codecov by Harness.
📢 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.

[Copilot]

🟡 Changes recommended

It removes the only test exercising the capacity_growth_limit > 1.0 path without providing a deterministic replacement to retain minimal coverage for that branch.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR removes a flaky unit test in investment_constraints that asserts against log output via logger.pop(), which can become non-deterministic under parallel test execution.

Changes:

• Removed logtest::Logger usage from the module’s test suite.
• Deleted validate_capacity_growth_limit_warning, which asserted on an emitted warning log message.

File summaries

File	Description
src/input/process/investment_constraints.rs	Removes a non-deterministic log-asserting test (and its logger import) to stop sporadic CI failures.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.30.

Benchmark suite	Current: 9b9428e	Previous: 7cd741d	Ratio
select_best_assets_sequential/01	9453340.012152778 ns	7245930.892045455 ns	1.30

This comment was automatically generated by workflow using github-action-benchmark.

[dc2917]

Yeah, they do talk about this in the logtest docs, and recommend separating any logging tests into a single file containing a single #[test] block, which seems a bit annoying.

Presumably you can iterate through the logger's contents and check that the expected entry is there, rather than being the last.

As you say, it's a pretty basic test of a fairly minor feature, so might as well delete. But as it's the only use of logtest, might as well delete that from Cargo.toml

[tsmbland]

Presumably you can iterate through the logger's contents and check that the expected entry is there, rather than being the last.

I considered that, but it's a bit messy, and what if another test just happened to produce the same message (not likely in this case, but in general)

As you say, it's a pretty basic test of a fairly minor feature, so might as well delete. But as it's the only use of logtest, might as well delete that from Cargo.toml

Done!