Add test extension [ECR-2634]

[MakarovS]

Overview

Use Extension to get rid of repetitive views instantiation.

---

See: https://jira.bf.local/browse/ECR-2634

Definition of Done

• There are no TODOs left in the code
• Change is covered by automated tests
• The coding guidelines are followed
• Public API has Javadoc
• Method preconditions are checked and documented in the Javadoc of the method
• Changelog is updated if needed (in case of notable or breaking changes)
• The continuous integration build passes

[dmitry-timofeev]

1. I would make this extension create things on demand (i.e., create a snapshot or fork only if it is needed).

2. I doubt that it will work correctly with parallel tests, as it uses its fields to store per-test data. As extensions are usually instantiated once, it is not recommended: https://junit.org/junit5/docs/current/user-guide/#extensions-keeping-state and http://blog.codefx.org/design/architecture/junit-5-extension-model/#Stateless

It seems that you can use a CloseableResource in a Store to be notified when the context has finished: https://junit.org/junit5/docs/current/api/org/junit/jupiter/api/extension/ExtensionContext.html#getStore(org.junit.jupiter.api.extension.ExtensionContext.Namespace)

As the store is hierarchical, I guess the following will also work with the store (but please confirm in the docs and test):

1. A MemoryDb is injected in @BeforeEach; the setup method writes some stuff into a fork and merges it into the database.
2. A Snapshot is injected in @Test; the MemoryDb is queried from the store under the hood, hence the snapshot contains the stuff written in @BeforeEach.

You may also see https://github.com/junit-pioneer/junit-pioneer/blob/master/src/main/java/org/junitpioneer/jupiter/TempDirectory.java extension as an example.

[DENMROOT]

It seems after @dmitry-timofeev comments that this extensions adds more headache than actual benefits. Can we keep it simple at least in tests ?

[MakarovS]

@DENMROOT at the moment we have 59 try(MemoryDb and/or Cleaner) blocks in our tests which would be omitted if we implement this extension. It's debatable, but this extension would allow us to get rid of about 300 lines of code

@dmitry-timofeev should we also move

static {
    LibraryLoader.load();
  }

to this extension? That'd reduce boilerplate code in tests that use views

[dmitry-timofeev]

LibraryLoader

It is a good idea, if the library is available at extension instantiation time.

---

Regarding the possible complexity, we can't estimate it until we see. As this is a spike, the goal here is to explore this solution and then decide if it makes sense to use it. It seems that the extension model has some nice built-in things (like a hierarchical store with built-in lifecycle management of stored objects) that might possibly make the implementation quite consice.

at the moment we have 59 try(MemoryDb and/or Cleaner) blocks in our tests which would be omitted if we implement this extension.

On top of reducing repetition and noise in our tests, this extension might be an early prototype of Java Binding Testkit, i.e., an extension that we will distribute to end-users to make testing of services easier.

[dmitry-timofeev]

Good, thanks. There are a couple of questions/improvements, but I think they can be resolved during implementation. Please put them in a ticket so that we can get back to them.

[dmitry-timofeev]

That's just for illustrative purposes, i.e., it's not needed for the below test to work, is it?

[MakarovS]

Yes, it's not needed, just a demonstration that we can inject parameters into @BeforeEach as well

[dmitry-timofeev]

Can't this code use just a single constant namespace, because the store is hierarchical:

- FooTest
  - setUp(MemoryDb) <- If we resolve MemoryDb here to instance A, 
    - test1(MemoryDb) <- … the same instance A will be accessible here
    - test2(Snapshot) <- … and this snapshot must come from the same instance A too
- BarTest
  - test3(MemoryDb, Fork) <- Has a distinct store from FooTest, hence will create a new MemoryDb instance B and get a fork from it.

[MakarovS]

Yes, we could do it either way

[dmitry-timofeev]

It probably makes sense to bind a Fork/Snapshot to a Cleaner, not a database.

[dmitry-timofeev]

This can probably simplify things too (with hierarchical store), as before each callback will no longer be needed:

MemoryDb memoryDb = store.getOrComputeIfAbsent(MEMORY_DB_KEY, CloseableMemoryDb::new);
if (requestedType == MemoryDb.class) {
  return memoryDb;
}
// …

It seems natural to allow to inject the same memoryDb as in setup method (which merges some data), but shall we also allow reusing snapshots and forks in lower levels of the lifecycle hierarchy? If we do not, which seems like a reasonable behaviour, then how do we destroy a cleaner exactly after the setUp?

[MakarovS]

Closing, as I don't think this is needed after TestKit extension implementation - #913.