+1 on merging #1621 soon. It's a large change so could easily occur code conflict.
-Rui On Tue, Dec 3, 2019 at 10:42 AM Andrei Sereda <[email protected]> wrote: > Thanks Vladimir. Most of the changes look find to me (left a small comment > in PR) > > Once merged, I'll take care of remaining JUnit4 rules for > Cassandra/ElasticSearch/Mongo etc. > > > On Tue, Dec 3, 2019 at 1:23 PM Vladimir Sitnikov < > [email protected]> wrote: > > > Ok, I think PR1621 is ready to go: > > https://github.com/apache/calcite/pull/1621 > > I'm inclined to merge it shortly > > > > The PR converts most of the tests, and by default, it allows JUnit5 only. > > > > Certain modules still have "non-trivial" JUnit4 code, and there was an > > agreement to convert them later. > > The modules receive junit4 dependency via junit4=true in > gradle.properties. > > > > --- > > > > The most "controversial" change from my point of view is the removal of > > *Suite* classes. > > In other words, I have dropped classes like > > CalciteSuite, FileSuite, PlusSuite, and so on. > > > > I guess it is OK, as both Gradle and JUnit5 provide lots of ways to > launch > > tests (e.g. tags are good). > > The good thing about the removal is it would reduce the number of > > merge-conflicts, > > and it would simplify adding new test classes. > > > > --- > > > > The most notable difference between junit4 and junit5 .assert* is that > > junit4 has (message, expected, actual) > > argument order while junit5 uses (expected, actual, message). > > The latter is better when message is big (e.g. concatenated), and junit5 > > allows to use lambda, > > so the message is built only when assertion fails. > > > > I've performed argument shuffling with IntelliJ IDEA's "structural search > > and replace", so it should be OK. > > > > Vladimir > > >
