After a month or so of work I have a PR for review. See https://github.com/apache/calcite/pull/2685/ <https://github.com/apache/calcite/pull/2685/>.
The PR is huge (80 commits, 127 files changed, 20k lines added/removed) so may be hard to digest. I recommend that you read the commit log to understand the various refactorings. Note that SqlOperatorBaseTest both moved (from main to testkit) and changed name (to SqlOperatorTest) so diffing it might require some ingenuity. At this point I am especially interested in high-level comments (Does the test/fixture/tester/factory split work? Are there any other candidates for this refactoring?). If you have a lot of low-level comments (e.g. spelling mistakes) just send me a PR. It’s probably several days before I will squash, rebase and consider merging to main. Julian > On Nov 22, 2021, at 4:12 AM, Stamatis Zampetakis <[email protected]> wrote: > > Hello, > > I generally agree with the problems and goals set by CALCITE-4885 so > definitely worth pushing this forward. > > I also like the ideas for being compatible with JUnit5 extensions etc., but > I guess it could be something to address later one and does not need to be > part of CALCITE-4885. > If and when somebody comes with a concrete proposal we can evaluate this. > > Same reasoning goes for AssertJ; it may be beneficial for the project but > let's evaluate it separately. > > If I understand well, both AssertJ and extensions via JUnit5 may come in > conflict with the new APIs exposed by CALCITE-4885 so it would be nice if > we can advance all these in the same release. > > Last I wanted to mention that although people consuming Calcite releases > may not care much about big changes in the testing code, those who maintain > forks of the main repo will have a few more challenges to address when they > cherry-pick changes. > This is not an argument to push back the feature but just an additional > element for completing the picture. > > Best, > Stamatis > > On Wed, Nov 17, 2021 at 7:15 AM Vladimir Sitnikov < > [email protected]> wrote: > >> Jacques>This sounds like it will mean we will need to make calcite-core >> test artifacts available >> >> Test artifacts publication is yet another anti-pattern just like "base test >> class". >> This change has been discussed: >> https://lists.apache.org/thread/fz96p94h016p11g777otqntjxg2oxgh1 >> >> If you want to depend on a class from tests, consider moving it to /testkit >> module: >> >> https://github.com/apache/calcite/tree/0899e6c157632ba1c5369a942cfe2be15fb4ed9f/testkit >> >> Jacques>We should think about the rules around Kotlin >> >> What happens in calcite-core/tests stays in calcite-core/tests :) >> >> It is reasonable to assume that testkit module would have dependencies, >> and testkit would provide API that is usable from Java and other JVM >> languages. >> >> In that regard, Kotlin dependency in testkit is not much different from >> Quidem or commons-lang3. >> Consumers might use Quidem if it fits just like they could use Kotlin if it >> fits. >> >> Vladimir >>
