I think it would be good in future junit5 upgrades to *not* use the jupiter assertions but instead move to assertj
The jupiter assertions are backwards-incompatible set of changes which add needless engineering effort. https://junit.org/junit5/docs/5.0.1/api/org/junit/jupiter/api/Assertions.html take assertEquals(); used across our code because it automatically generate good assertions provided you get expected and actual in the right order. And you can optionally add more text at the beginning, e.g assertEquals("hint", expected, actual) Only now the jupiter one changes the signature to assertEquals(expected, actual, "hint") So everything needs changing. This is not only needless work -it replicates the needless work on every backport *and* runs the risk that if hey test case is comparing strings we may not even notice that there are reordering of the hint has happened. I think it's a real shame given how absolutely stable JUnit has been right since they first wrote it in 2020. It and the first CI tool, CruiseControl, profoundly transformed software development. Could you imagine what's the quality of the entire Apache open source code base would be like without JUnit; without CI testing running continually? It would be awful. Do we actually need to move off the org.junit.Assert class for our asserts? As if it is not going to be deleted then we can stay with it for all existing tests. And for new tests, we should embrace AssertJ and its assertThat() which * can coexist with classic junit Asserts * is extensible (see IOStatisticAssertions) * chains really well * creates wonderful exception messages, especially if you have an assertion that a list has a size. contains etc // takes an iterator, maps it and assets the result contains for values (equality test) in any order Assertions.assertThat(fetcher.getFileStatuses()) .describedAs("result of located scan") .flatExtracting(FileStatus::getPath) .containsExactlyInAnyOrder( emptyFile, subdirFile, subdir2File1, subdir2File2); // instanceof plus extraction of new value for a new chain of asserts. // exception string uses String.format() sequence with robust evaluation and called only on failure Assertions.assertThat(source) .describedAs("metric source %s", metricSourceName) .isNotNull() .isInstanceOf(WeakRefMetricsSource.class) .extracting(m -> ((WeakRefMetricsSource) m).getSource()) .isSameAs(instrumentation); See: lovely. And because we can add an import of assertJ on older versions, easily backported. while moving all existing asserts to assertJ is an excessive action, embracing it for new tests is something people should be doing. Proposed * -1 to moving existing code to jupiter asserts *unless someone has a good reason* * +1 to adding new assertions in AssertJ, ideally with good describedAs * +1 to all classes having toString() values which are good in assertions and logs while not leaking secrets I'd also encourage new tests to extend AbstractHadoopTestBase which has a global timeout (controllable via a property) and names the test thread with the current method for useful log output. The older HadoopTestBase class extends org.junit.Assert to give you the older asserts. If we have to move off org.junit.Assert then we could add our own methods to that class so we can leave our own code unchanged. I don't want to do that, but compared to the effort of moving every single assert, justified. On Tue, 21 Feb 2023 at 06:24, Ayush Saxena <ayush...@gmail.com> wrote: > I think it is 2.5 months and the Junit upgrade tests are still in a mess[1] > There was an attempt as part of MAPREDUCE-7428, but that didn't fix > everything or some other commits induced these problems again. I don't > think anyone is actively chasing that either. > > Can add Junit-vintage dependencies here and there and most probably can > fix this, but last time the approach taken was different, so not exploring > in that way. > > Planning to backtrack and revert/reopen all the Junit upgrades tickets > till I get a green build. Will hold a minimum of 24 hrs, next whenever I > find some time, will revert all of these. > > Shout out if anyone has concerns around this, or is working on the fix > > ++ @Akira Ajisaka <aajis...@apache.org> / @mapreduce-dev > <mapreduce-...@hadoop.apache.org> > > -Ayush > > [1] > https://ci-hadoop.apache.org/view/Hadoop/job/hadoop-qbt-trunk-java8-linux-x86_64/1142/testReport/junit/org.apache.hadoop.mapreduce.v2.hs/TestJobHistoryEvents/testEventsFlushOnStop/ > > > On Fri, 23 Dec 2022 at 08:50, Ayush Saxena <ayush...@gmail.com> wrote: > >> Have we stopped >>> with the java8 builds? >> >> >> Nopes, The answer is here just cross posting: >> https://github.com/apache/hadoop/pull/5226#issuecomment-1354964948 >> >> Regarding dropping Java-8 and adapting Java-11, We just have runtime >> support for Java-11 in hadoop, the compile support ain't there, it is being >> tracked here: >> https://issues.apache.org/jira/browse/HADOOP-16795 >> >> Some issues are there, one with Jersey I know and may be a couple of more. >> >> -Ayush >> >> On Fri, 16 Dec 2022 at 20:07, Steve Loughran <ste...@cloudera.com.invalid> >> wrote: >> >>> OK, it's a JDK bug >>> >>> both the java8 and java11 javadocs are now using java11. Have we stopped >>> with the java8 builds? >>> >>> as i am happy with that, we just need to make an explicit declaration and >>> wrap up of anything outstanding. >>> >>> >>> >>> On Thu, 15 Dec 2022 at 22:30, Ayush Saxena <ayush...@gmail.com> wrote: >>> >>> > Thanx Ashutosh, Let me know if you need any help there. >>> > >>> > Got some time to recheck the Javadoc stuff, it seems like a JDK bug >>> > https://bugs.openjdk.org/browse/JDK-8295850 >>> > >>> > more details over here: >>> > >>> https://github.com/apache/hadoop/pull/5226#pullrequestreview-1220041496 >>> > >>> > -Ayush >>> > >>> > On Mon, 12 Dec 2022 at 19:46, Ashutosh Gupta < >>> ashutoshgupta...@gmail.com> >>> > wrote: >>> > >>> >> Thanks Ayush for pointing out the failures related to the Junit 5 >>> >> upgrade. As I have closely worked in upgrading Junit 4 to Junit 5 >>> >> throughout the hadoop project. I will create a JIRA for these >>> failures and >>> >> fix them on priority. >>> >> >>> >> -Ashutosh >>> >> >>> >> On Mon, Dec 12, 2022 at 1:59 PM Ayush Saxena <ayush...@gmail.com> >>> wrote: >>> >> >>> >>> Try to fix in the same way it was done here and couple of similar >>> PRs: >>> >>> https://github.com/apache/hadoop/pull/5179 >>> >>> >>> >>> There are a bunch of PRs in yarn getting the similar error fixed >>> module >>> >>> wise, the problem would be there in many other modules as well... >>> >>> >>> >>> The daily JDK-11 build also shows that failure here: >>> >>> >>> >>> >>> https://ci-hadoop.apache.org/job/hadoop-qbt-trunk-java11-linux-x86_64/410/artifact/out/patch-javadoc-root.txt >>> >>> >>> >>> BTW. the daily build is also broken with some whooping 150+ failures >>> >>> >>> >>> >>> https://ci-hadoop.apache.org/view/Hadoop/job/hadoop-qbt-trunk-java8-linux-x86_64/1071/testReport/ >>> >>> >>> >>> Mostly some Junit upgrade patch being the reason. >>> >>> >>> >>> -Ayush >>> >>> >>> >>> On Mon, 12 Dec 2022 at 18:46, Steve Loughran >>> <ste...@cloudera.com.invalid >>> >>> > >>> >>> wrote: >>> >>> >>> >>> > yetus is now reporting errors on our @InterfaceAudience tags in >>> java8 >>> >>> and >>> >>> > java11 javadoc generation >>> >>> > https://github.com/apache/hadoop/pull/5205#issuecomment-1344664692 >>> >>> > >>> >>> > >>> >>> >>> https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5205/2/artifact/out/branch-javadoc-hadoop-tools_hadoop-azure-jdkUbuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04.txt >>> >>> > >>> >>> > it looks a bit like the javadocs are both being done in the java11 >>> >>> version, >>> >>> > and is is unhappy. >>> >>> > >>> >>> > any suggestions as to a fix? >>> >>> > >>> >>> >>> >> >>> >>
