Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unit tests on Large Craft Bays #4059

Merged
merged 1 commit into from
May 8, 2024

Conversation

AaronGullickson
Copy link
Member

@AaronGullickson AaronGullickson commented May 7, 2024

It works! All green. I think some of this is correcting mistakes from my last PR plus a few other tests that were failing.

Comment on lines -1541 to +1540
bayWeapons.add(weaponBay);
bayWeapons.add(mounted);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In several cases, this was the problem - the weaponBay was being added as a weapon of itself, instead of mounted which is the key equipment being tested.

Comment on lines +211 to -215
ammoBin.setUnit(unit);

// Set the bay as if we're in deserialization code
ammoBin.setBay(bayNum);

ammoBin.setUnit(unit);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit has to be set before you add the bay number.

Comment on lines 278 to +279
when(entity.getWeaponBayList()).thenReturn(weaponBays);
when(entity.whichBay(equipmentNum)).thenReturn((WeaponMounted) weaponBay);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this solution because entity.whichBay should already work because it calls entity.getWeaponBayList which is mocked in line 278. However, on the debugger, that method was returning null within the whichBay function. I think its ok?

//bayWeapons.addElement(33);
bayWeapons.add(weaponBay);
bayWeapons.add(mounted);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what 33 was supposed to be as this equipment number is not used in this test and its supposed to be testing a weapon bay with only one weapon.

@AaronGullickson AaronGullickson requested a review from Sleet01 May 7, 2024 06:13
@AaronGullickson AaronGullickson added the Tests Issues or Pull Requests related to the project tests label May 7, 2024
Copy link
Member

@SJuliez SJuliez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works, it works.

@AaronGullickson AaronGullickson merged commit bc0e140 into MegaMek:master May 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tests Issues or Pull Requests related to the project tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants