Java - Cars, Assemble - provided tests improvement suggestion

pdmoore · October 9, 2023, 9:59pm

I just worked through the Cars, Assemble exercise and encountered something that I think of as a unit test anti-pattern.

In the current implementation the assertions that compare doubles are in the form:
assertThat(Math.abs(carsAssemble.productionRatePerHour(9) - 1591.2) < epsilon).isTrue();
which means when the test fails the student gets an unhelpful message:
"Expecting: <false> to be equal to <true> but was not."

I thought it was standard to use the AreEquals double-with-delta form:
Assertions.assertEquals(1591.2, carsAssemble.productionRatePerHour(9), epsilon);
which reports the same failure in a more informative way:
Expected: 1591.2
Actual: 1989.0

The C# track does seem to have the more informative test pattern:
Assert.Equal(1591.2, AssemblyLine.ProductionRatePerHour(9), precision: 1);

I’ll make the changes if it’s worth updating - I did convert the tests locally when solving to get the added information anyways

ErikSchierboom · October 13, 2023, 8:54am

Yes please! That would definitely be very helpful.

pdmoore · October 17, 2023, 5:25pm

Apologies for the delay, life & stuff…

Here’s the proposed change:

github.com/exercism/java

Update CarsAssembleTest.java

exercism:main ← pdmoore:patch-3

opened 05:17PM - 17 Oct 23 UTC

pdmoore

+7 -6

Tests for Task 1 used a boolean comparison to determine whether tests pass. Repl…aced those with more idiomatic assertEquals(double, double, epsilon) form. When the new style of test fails the user is shown the expected value and the actual value: Expected :1591.2 Actual :1989.0 The previous implementation showed the user the less clear: Expected :true Actual :false The test implementation being replaced required the comparison to be strictly less than epsilon, not inclusive of epsilon. The JUnit assertEquals replacement check is inclusive of epsilon. OLD: Math.abs(value1 - value2) < epsilon) NEW: Math.abs(value1 - value2) <= epsilon Given an epsilon of 0.0000001d this is unlikely to see any existing solutions fail given the expected answers only check the tenths place. # pull request --- Reviewer Resources: [Track Policies](https://github.com/exercism/java/blob/main/POLICIES.md#event-checklist)

Note the difference in the epsilon boundary between current implementation and using assertEquals(double, double, epsilon). I don’t think it will cause any existing solutions to fail but honestly don’t remember the depths of double comparisons and if the existing epsilon is the right precision.
My gut is that the epsilon value doesn’t need to be 7 digits and for the values being calulcated in this exercise an epsilon of 3 or 4 digits would be precise enough.