I think it is a great idea to make the tests more accessible. A few comments to consider in your work (no response expected):
- I would vote to try to minimize the use of non-interface class inheritance , especially in downstream cases. For example, I would avoid a dominant path using base test subclasses in other projects. My experience on other similar projects is that using multi-level inheritance as a strategy makes things ultimately harder to compose. - This sounds like it will mean we will need to make calcite-core test artifacts available as part of distribution, something that doesn't happen currently <https://repo1.maven.org/maven2/org/apache/calcite/calcite-core/1.28.0/>. I was just trying to use SqlToRelTestBase.TesterImpl externally today and was disappointed to find out that maven doesn't hold the test classifier jars. - We should think about the rules around Kotlin, since it is used in the core test compilation cycle. Do projects need to depend on and/or adopt Kotlin to use these things? - I have zero opinion of AssertJ as relevant or a good idea. My last thought is that I sometimes worry that we're quick to build bespoke infrastructure within the project when there are good-enough primitives that we could leverage that are more ecosystem idiomatic. A few examples would be early json reading/writing, several relatively undocumented custom plugins in the gradle build, ImmutableBeans, excessive use of unusual reflection patterns, and use of Janino in non-performance-critical code paths. I just hope we do our best to use ecosystem idiomatic solutions here as much as makes sense. It improves approachability and helps when people are struggling with problems (since usually there is more content out there for people to figure things out on their own). Again, super glad this work is getting done. Thanks for taking the initiative Julian! On Tue, Nov 16, 2021 at 2:27 PM Julian Hyde <[email protected]> wrote: > I am working on https://issues.apache.org/jira/browse/CALCITE-4885, > "Fluent test fixtures so that dependent projects can write parser, > validator and rules tests". > > I wanted to give you all a heads up, because it's going to change > quite a few lines of code. > > Here's the problem I'm trying to solve. Calcite defines APIs such as > RelRule, SqlOperator, and we encourage people to implement them in > their projects (not necessarily contribute them back). But experience > has shown that if you want to write, say, a planner rule, you'd better > write some tests for it. And to write tests, you need to sub-class > RelOptRulesTest. > > So, 4885 is about making it easier to write those kinds of tests. > > But - spoiler alert - subclassing the test classes isn't the way to > go. Your subclass will inherit all of the tests from the base class. > If you keep mutable state ("fixtures") in your test class, then > maintaining that state gets messy as more tests add more state. We > have added abstractions such as "tester" [1] but it's still pretty > complicated. > > I am refactoring the test along the following lines: > > Test classes (e.g. RelOptRulesTest) contain tests but have no state. > They have a "fixture()" method that returns a fixture. Often there is > also a "sql(String sql)" method that just does > "fixture().withSql(sql)". > > A fixture class (renamed from the helper classes called "Sql" in > various tests) has a lot of methods that allow you to test things. But > the only state in the fixture is a tester and a config. That state is > immutable; each time you set a property, you create a new fixture that > wraps a new copy of the config. > > A test config class is immutable and has lots of data fields. > > If the test uses reference files, the config will contain a > DiffRepository. Subclasses can use a different DiffRepository (mapping > to a different .xml file) but still use the same fixture > implementation. > > A tester implements the nuts and bolts (e.g. parsing SQL, converting > SQL to RelNode, planning). It has no state; the methods accept lots of > parameters, such as whether decorrelation is enabled, from the config. > There's usually only one implementation of tester, but you can > override for special reasons (e.g. an implementation of the parser > tester that converts the AST back to SQL and tries to parse it a > second time). > > With these changes, people will be able to create a test that > instantiates a fixture, calls some methods to configure it, and then > calls some methods to run their test. See FixtureTest and Fixtures > [2]. They can do that in any class, not necessarily a subclass of an > existing Calcite test. > > Our test framework is not covered by semantic versioning. People in > dependent projects who depend on our test framework may have to fix up > their tests when they upgrade. It shouldn't be too bad. > > In the bug Vladimir suggested that we take this opportunity to move > from Hamcrest matchers to AssertJ. I disagree. I'm not convinced that > AssertJ is so much better that moving to it is worth the disruption. > And in any case, this change is about figuring out the lifecycle (e.g. > how do I configure my test to add and remove rules from the planner, > when the planner doesn't exist until I'm half way through my test) and > I don't think AssertJ helps with that. > > I am going to make fixtures, configs and testers top-level classes > (i.e. not inner classes). It makes them easier to extend. > > I might make fixtures into interfaces whose methods all have defaults. > Then people can easily subclass fixtures; they could even inherit from > multiple fixtures. > > Let me know if you have ideas or concerns. I recommend that people who > have written tricky tests (e.g. TopDownOptTest) track this > development. > > Julian > > [1] > https://github.com/apache/calcite/blob/4b1e9ed1a513eae0c873a660b755bb4f27b39da9/core/src/test/java/org/apache/calcite/test/SqlToRelTestBase.java#L568 > > [2] > https://github.com/julianhyde/calcite/commit/0286f78b0db0e364f3225f004b32853de2685a39 >
