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

CodeUtilities: Add Unit Testing #4624

Merged

Conversation

Windchild292
Copy link
Contributor

Commit 1 adds unit tests, commit 2 expands Javadocs and fixes a rounding bug in MathUtility's long lerp.

@Windchild292 Windchild292 added Bug Tests Issues or Pull Requests related to the project tests labels Jul 14, 2023
@Windchild292 Windchild292 self-assigned this Jul 14, 2023
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.

All good. Is there a difference between compareNullable and Objects.compare?

@Windchild292
Copy link
Contributor Author

Objects.compare doesn't handle one parameter being null and the other being not null... just both having values or both being null.

// The order of operations is important here, to not lose precision
return min * (1L - f) + max * f;
return Math.round(min * (1L - f) + max * f);
Copy link
Member

Choose a reason for hiding this comment

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

Every time I see a change to a "deep library" function like this, I get scared. Do we have any idea where it's used? And if so, can we get a short write-up of stuff that can be tested from the user side to see if the change "affects" anything?

Copy link
Member

Choose a reason for hiding this comment

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

According to my IDE the lerps are not currently used at all. I guess it's safe to merge.

@SJuliez SJuliez merged commit ae3fd3a into MegaMek:master Jul 21, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Tests Issues or Pull Requests related to the project tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants