fred icon indicating copy to clipboard operation
fred copied to clipboard

Junit4

Open vwoodzell opened this issue 3 years ago • 4 comments

I upgraded all of the junit3 tests to junit4, for ease of maintenance. There are lots of affected files, but the changes are mostly small. The last commit is a huge pile of trivial changes, mostly carried out by a script. The other commits have the more interesting changes.

I verified that I got 985 test cases run, before and after these changes.

vwoodzell avatar Sep 03 '22 00:09 vwoodzell

I really like the idea of getting rid of the old JUnit 3 stuff… however, for JUnit 4 I would love to see more Hamcrest usage, you know?

assertThat(foo.getValue(), equalTo("someValue"));

instead of

assertTrue(foo.getValue().equals("someValue"));

However, I’m aware this may very well create a hugh rabbit hole (first you’re going to write methods that match more complicated objects, then you start creating custom matchers for those objects, and then you’ll create a meta-object matcher which will just boggle everybody’s mind…) so, I don’t know, maybe postpone that? No idea. ¯\_ (ツ)_/¯

Bombe avatar Sep 05 '22 11:09 Bombe

I never paid much attention to hamcrest before, but it does seem like it could be pretty nice. There are definitely some things that we could clean up using that.

I would not make it part of this PR, though. This one's already cumbersome to look through, so I want to keep it as simple as possible. For the most part, the assertions aren't touched. (The exceptions to that are some floating-point comparisons and a conflicting method name.)

vwoodzell avatar Sep 05 '22 16:09 vwoodzell

Okay, so I just ran the old tests on next and Gradle reported 970 tests but on this branch it’s 980. I’m guessing some JUnit 3 constructs are counted differently and in any case it’s not fewer tests as before, so… 😁

(Also, holy crap, some of those tests are horrible…)

Bombe avatar Sep 10 '22 20:09 Bombe

The 10 new ones are probably from TagVerifierTest. That has 10 test methods; it was a nested class before; and the gradle script explicitly ignores tests in nested classes.

vwoodzell avatar Sep 14 '22 13:09 vwoodzell