Skip to content

Conversation

thiakil
Copy link
Member

@thiakil thiakil commented Jul 13, 2024

Changes proposed in this pull request:

Change FloatingLong to long

@pupnewfster pupnewfster marked this pull request as ready for review July 17, 2024 22:24
@@ -128,43 +101,32 @@

private interface Unit {

String getSymbol();
Object getSymbol(boolean singular);

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'singular' is never used.
double defaultConversion = 2.5;
double inverseConversion = 1 / defaultConversion;
int capacity = 1_000;
int convertedCapacity = (int) (capacity / defaultConversion);
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 think for a test, this should be a fixed value, and the calculation in a comment. Personally I'd also put them in screaming snake case for readability

//Ensure the config starts out at the default value
.thenExecute(() -> MekanismConfig.general.forgeConversionRate.set(defaultConversion))

// WRAPPING STRICT ENERGY TO FORGE ENERGY
Copy link
Member Author

Choose a reason for hiding this comment

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

these test should have extra comments detailing why they're doing what they're doing. e.g. "insert more than converted capacity", "ensure capped at converted amount"

int accepted = handler.receiveEnergy(capacity, false);
helper.assertValueEqual(accepted, 0, "accepted energy");
//Note: Needed energy should be 1 even though we can't accept it
helper.assertValueEqual(handler.getEnergyStored(), convertedCapacity - 1, "stored energy");
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 would have done this the other way. Calculation should go in the first argument, expected result should be 1

int accepted = handler.receiveEnergy(capacity, false);
helper.assertValueEqual(accepted, 0, "accepted energy");
})
//Test against a small empty container
Copy link
Member Author

Choose a reason for hiding this comment

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

test what against an empty container. If 1RF is 2.5 Joules, shouldn't it store 2 Joules?

helper.assertValueEqual(extracted, 0, "extracted energy");
int accepted = handler.receiveEnergy(capacity, false);
helper.assertValueEqual(accepted, convertedCapacity, "accepted energy");
helper.assertValueEqual(handler.getEnergyStored(), convertedCapacity, "stored energy");
Copy link
Member Author

Choose a reason for hiding this comment

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

these checks should not use wrappers.
interact via the wrappers, check with the unwrapped storage. Applies to all these tests

@pupnewfster pupnewfster merged commit 9c21d94 into 1.21.x Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants