You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@calcite.apache.org by Julian Hyde <jh...@apache.org> on 2021/11/16 22:26:52 UTC

[DISCUSS] Refactoring tests

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