brson · GitHub

Feature Request

Is your feature request related to a problem? Please describe:

With a second storage engine nearly merged in #8430, and raftstore is almost entirely parameterized over engines, we are going to soon need a way to run our test suite with more than one engine.

Describe the feature you'd like:

There are a number of ways to go about making the test suite compatible with multiple engines, and in the long term the strategy we use might be different than in the short term.

Since the test suite is spread over a lot of different crates and currently assumes a single engine, I think we should do the simplest thing possible right now.

What I suggest for now is this:

  • Create a new engine, TestEngine, in engine_test.
  • Make this engine an enum of every other engine, currently RocksEngine, PanicEngine and SkiplistEngine.
  • Create an engine constructor that selects the engine based on a TIKV_TEST_ENGINE environment variable.
  • Slowly convert tests to use the TestEngine constructor instead of the various RocksEngine constructors.

This will let the test suite continue to operate essentially as-is while other engines are being implemented,
while also cleaning up the test engine constructor code such that alternative solutions can be explored later.

Describe alternatives you've considered:

The obvious downside to this approach is that every method in engine_test will be a match statement.

This has two big implications:

  • Maintenance will be tedious, though trivial
  • Every method has an extra branch

That extra branch isn't a big deal for unit tests, but could have impact on benchmarks.

Some alternatives:

  • Again create a TestEngine, but use compile-time flags to decide which engines to use, removing the extra dispatch layer
  • Investigate solutions for parameterized tests in Rust and convert the test suite to use engine-parameterized tests.

Ultimately, it may be best to use some macro-based solution to writing paramaterized tests, but it's not obvious to me that there are solutions robust enough for our purposes right now, or that it is worth the effort to begin parameterizing the entire test suite right now.

Teachability, Documentation, Adoption, Migration Strategy:

This change only affects developers. The main way to teach this is by example. As the test suite is converted to the new idiom, adjacent code should naturally adopt those idioms. Once an entire test suite is converted to use engine_test instead of engine_rocks, then the engine_rocks dependency will be removed, forcing future maintainers to use engine_test.

We'll need to add some developer documentation about the environment variable and how to write engine-agnostic tests.

cc @5kbpers @hicqu what do you think?

cc #6402

Read the original on github.com ↗