Improve asset decommission behaviour

[Aurashk]

Description

Improve asset decommission to make more sense by enforcing decommission year == min(max decommission year, requested decommission year)

Requires review - see comment below.

Fixes #867

Type of change

• [ X] 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
• The documentation builds and looks OK: $ cargo doc

Further checks

• [ X] Code is commented, particularly in hard-to-understand areas
• [ X] 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 85.52%. Comparing base (e8f72c7) to head (403ead6).
⚠️ Report is 25 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
+ Coverage   85.18%   85.52%   +0.33%     
==========================================
  Files          50       50              
  Lines        5300     5387      +87     
  Branches     5300     5387      +87     
==========================================
+ Hits         4515     4607      +92     
+ Misses        565      560       -5     
  Partials      220      220              

☔ 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]

Please review:

• Are the csv files in tests/data/ just the expected outputs when you run the model? Since the decommission years are expected to be different under certain conditions with this change I simply updated the files with the new outputs to enable the regression tests to pass.

• I'm not sure about this ProcessBuilder I added in the fixtures belongs there. For the test I added I needed to be able to set the lifetime of the process associated with the asset. The existing process fixture didn't seem to work for this since the lifetime is immutable. The builder seemed like the most flexible approach to make a custom process for your test but open to suggestions on better ways to do this.

[tsmbland]

I think the actual change to the code looks good! (Although I'm going to double check with Adam that this is actually what we want)

I think the new test is probably overkill. I would personally just keep the existing test as it is and add one more scenario where you use a year way into the future like 9999, and check that the decommission year of the asset equals asset.max_decommission_year().

That said, maybe there's some merit to this ProcessBuilder idea... I could definitely see this being useful elsewhere, but I wouldn't bother with it if we're only going to use it for one test.

Are the csv files in tests/data/ just the expected outputs when you run the model? Since the decommission years are expected to be different under certain conditions with this change I simply updated the files with the new outputs to enable the regression tests to pass.

Yes. We have a script regenerate_all_data.sh to do this for you, if you weren't already using that. Since the results have now changed on main, you'll have to merge/rebase with main and regenerate the files again

[tsmbland]

I think the actual change to the code looks good! (Although I'm going to double check with Adam that this is actually what we want)

Have checked with Adam and this is indeed the correct approach for now

[dalonsoa]

This LGTM, although I agree with @tsmbland in that, only for this test, the ProcessBuilder might be a bit of an overkill.

[alexdewar]

This is good!

Agree with the others that the ProcessBuilder is a bit overkill, though thanks for writing tests for it! Maybe just add another test case to the existing tests as @tsmbland suggested.

In terms of the lifetime not being mutable, that's because the ProcessParameter field of Assets is an Rc type, but you can make it mutable with Rc::get_mut (or Rc::make_mut). Not saying you have to do this, but that's how you can mutate Rc things if you really need to.

If you have just installed, you can run just regenerate-test-data or call the script directly as @tsmbland said.

[alexdewar]

PS -- @Aurashk remember to assign yourself to issues and mark them as in progress etc. when you take them, so that someone else doesn't start working on it. I've done this now.

[Aurashk]

Thanks all! It turns out I could change the lifetime in the existing process fixture from 1->5 without breaking anything which let me write the test in a much more straightforward way. Thinks it's ready to check again

[tsmbland]

Looks good!

[alexdewar]

Much cleaner. Good work!

As discussed, the CI should sort itself out once you update this branch, then I think we're good to merge.

[alexdewar]

@all-contributors please add @Aurashk for code

[allcontributors]

@alexdewar

We had trouble processing your request. Please try again later.

[alexdewar]

How about now....

@all-contributors add @Aurashk for code

[allcontributors]

@alexdewar

We had trouble processing your request. Please try again later.

[alexdewar]

I tell you, you can't get the bots these days 🤷

[alexdewar]

Maybe having a rest over the weekend will have fixed it?

@all-contributors add @Aurashk for code

[allcontributors]

@alexdewar

We had trouble processing your request. Please try again later.

[alexdewar]

I guess I'll have to do it manually then 🤦