search

Mojang / brigadier

Brigadier is a command parser & dispatcher, designed and developed for Minecraft: Java Edition.
MIT License
3.41k stars 392 forks source link

Tests (where relevant) don't actually make sure that an exception was thrown. #106

Open SoniEx2 opened 2 years ago

SoniEx2 commented 2 years ago

Tests like these:

https://github.com/Mojang/brigadier/blob/cf754c4ef654160dca946889c11941634c5db3d5/src/test/java/com/mojang/brigadier/StringReaderTest.java#L239-L247

will actually pass if no exception is thrown.

Pokechu22 commented 2 years ago

According to this random article I found, JUnit 5 has an assertThrows that returns the exception if a matching one is found (and fails otherwise), which is probably the ideal thing for this kind of test. JUnit 4 has the ExpectedException rule, but I don't think that it would be usable for this purpose.

The easiest approach, and one that already seems to be used by some of the existing tests, is to call fail() after the code that should throw the exception. That's easy enough that I'll look into making a PR for it.

Marcono1234 commented 2 years ago

JUnit 4 also has an assertThrows method since version 4.13, see https://github.com/junit-team/junit4/wiki/Exception-testing#using-assertthrows-method

Also Brigadier uses the old Maven artifact coordinates for JUnit: junit:junit-dep:4.11 It should switch (and update) to junit:junit:4.13.2