Schamschi commented on issue #48: [WIP] perform the actual migration to JUnit 5 URL: https://github.com/apache/commons-numbers/pull/48#issuecomment-500389290 @grimreaper Why did you agree to wait for the pull request #36 to be merged (or closed unmerged) and then go ahead and refactor `FractionTest` anyway? Not only did I replace several `Assert.assertEquals` pairs comparing the numerator and denominator separately with a single call to the method `assertFraction`, which already reduces the number of `Assert` references drastically, but I also added anonymous blocks in the test methods to make them a little less messy, and resolving the conflicts arising from the introduction of these anonymous blocks in a merge or rebase is _very_ laborious – I already did it once yesterday when updating my pull request to be compatible with the changes introduced by the merge of fraction-dev, and the thought of doing it a second time is rather off-putting. Besides, I don't think `assertEquals(a, b)` and `assertTrue(a.equals(b))` are 100% interchangeable, because, the way I see it, they reflect different intentions, even if they may be functionally equivalent most of the time. Judging by its design and the names for the parameters, `assertEquals(a, b)` is meant for situations where you have a pre-computed (at compile- or runtime), "expected" value, against which an unknown, "actual" value computed at runtime is checked. But there may be situations where both values are unknown, and one is not necessarily more correct than the other, for example, when comparing two hash codes, or when testing the `equals` method itself. Speaking of testing the `equals` method: Although not done in this case as far as I am aware, I could imagine a scenario where one would want to test of the symmetry of the `equals` method by testing both `a.equals(b)` and `b.equals(a)`. `assertEquals` does not specify whose object's `equals` method is called, so replacing `assertTrue` constructs with `assertEquals` constructs here seems inappropriate. A situation related to this can be found in the class `BigFractionTest`, line 601, where the original code called the `equals` method of `BigFraction` with a `Double` argument: > `Assert.assertFalse(zero.equals(Double.valueOf(0)));` So in this case, it is crucial for the validity of the test that the `equals` method of the `BigFraction` instance `zero` is called, and not the `equals` method of the `Double`. However, in its current version, the `equals` method of `Double` is called: > `Assertions.assertNotEquals(0.0d, zero);` `0.0d` is implicitly boxed to `Double`, and since `assertNotEquals` calls the `equals` method of the first argument (which is an implementation detailed and not documented), the test becomes useless. By this logic, there are situations where I find your conversion from `assertTrue` to `assertEquals` absolutely beneficial, for example, the `testParse()` method in the class `ComplexTest` from the "complex" module, or `testMath843()` in `PrecisionTest` in the "Core" module, thank you. Also, the original parameter order for `assertEquals` in SmallPrimesTest was correct, and your change to the file `ArithmeticUtilsTest` in the commit "Simplify junit" also has the parameters in the wrong order, but I don't have time to fix it now, maybe you can tend to it if you get back here before I do.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
