Fix: #7025 Fixed Mass Repair Dialog Throwing Exception When Run From Warehouse #7027
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7027 +/- ##
============================================
- Coverage 12.04% 12.04% -0.01%
+ Complexity 6891 6887 -4
============================================
Files 1102 1102
Lines 142196 142205 +9
Branches 21984 21985 +1
============================================
- Hits 17131 17122 -9
- Misses 123230 123250 +20
+ Partials 1835 1833 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Nice! Further reinforcing that we need more unit tests, as the tech sorting is not something currently covered by MRMSServiceTest.
I wonder if you could get the error triggering with new unit tests, and then verify your change fixed it....
Anyway, one comment, not something that needs fixed.
| } | ||
| // Nulls at the end | ||
| if (skill1 == null && skill2 == null) { | ||
| return 0; |
There was a problem hiding this comment.
If the skill is null for both should we try to compare their minutes remaining before saying they're equal?
There was a problem hiding this comment.
Pull Request Overview
This pull request addresses issue #7025 by refining the tech comparison logic in the mass repair dialog to resolve exceptions caused by mixed comparison methods. Key changes include improved null handling for skills, removal of outdated XP and elite checks in favor of comparing total final skill levels, and a fallback to minutes left when skills are tied.
Comments suppressed due to low confidence (1)
MekHQ/src/mekhq/service/mrms/MRMSService.java:1230
- [nitpick] The comment still refers to XP-based sorting, but the comparator now uses total final skill levels. Update the comment to accurately reflect that techs are being compared based on their final skill levels.
Sort the valid techs by applicable skill. Let's start with the least experienced and work our way up until we find someone who can perform the work. If we have two techs with the same XP, put the one with the more time ahead.
Fix #7025
We were running into comparison errors when comparing techs because we were using mixed comparison methods. Comparing both by experience level, and then if experience levels were equal by XP OR skill level (if Elite). Then finally available time.
This creates several issues:
To mitigate this issue I've changed up the comparison. We now only compare final skill levels followed by available minutes in the event that final skill levels are equal. I also added in better null handling, in the event we feed a null skill into the comparison (this can happen if the unit attached to the part doesn't have a tech skill).
The way the final skill level comparison works it is compares total skill level (factoring in all modifiers). This means that if we're comparing two techs we're sorting based on who has the highest skill. Not who has the highest experience level (green, regular, etc). This better handles irregular experience levels. Such as if the player modified their campaign options so that MekTechs hit Elite at a different time to AeroTeks, for example.
Suspected Cause
That MRMS is randomly breaking like this is concerning, but I suspect it's likely due to MRMS finally getting some attention after years of neglect. This is being compounded by two factors: with the mass adoption of ATOW skills level 4 is no longer 'Elite', it's 'Veteran'; there are now significantly more skill modifiers than when MRMS was developed.
Tests