[PersistableStateMigrations] Integration test with jest-snapshot approach

[Dosant]

What is a persistable state?

Recently, we had a bunch of issues around migrating saved objects with persistable state-owned by other plugins. One area for improvement that we identified is better functional testing. We are working on adding more specific function tests: #102667 and we plan to document steps on how to create such tests that use old saved objects with old states.

In addition to functional tests like in #102667, I want to suggest a low-level migrations testing system where it would be cheaper/faster to add new migrations tests:

1. This would be an API-level test. It would use saved_object/_migrate API which gets an old saved object and returns a new saved_object. We would like it to be an integration test, as it is hard to mock all the registries which are used when migrating a saved object with a persistable state.
2. As input we use a serialized saved object in the older format. We run a migration. We assert the result with an expected snapshot, a mechanism similar to jest snapshots.
3. To add a new saved object migration test, all developer needs to do, is:
   3.1. Create a new JSON file with a saved object in an older format
   3.2 Run a test suite once so that a snapshot is created. Review the snapshot.
   3.3. Commit both input and a snapshot.

The assumption is, that having such a system in place, more tests would be created, as it would be faster to add and easier to maintain than such UI tests. Such tests don't remove a need for UI tests, we would still have some UI tests that test migrations, but we won't be focused on those that much.

POC that implements such: #102745

• It auto-discovers files to tests a migrations on (auto-creates tests from input files)
• It reuses jest-snapshot package for snapshot feature.

So in that POC, to add a new migration test, all a developer needs to do:

1. Add a test like: test/api_integration/apis/migrations/tests/7_12_0/dashboard_by_value.json
2. Run a test suite
3. Review created .snap file
4. Commit the .snap file

Questions

I think the main general question, for now, is:
Do you think testing migration on this level: running Kibana / making an API call / asserting serialized saved object makes sense?

If yes, we can discuss and work on implementation separately, maybe, inside the pr: #102745

[streamich]

I like it, the only question I have is if the Core team wants to expose such an endpoint, if they don't maybe we can create _migrate route only when server is in test mode. Maybe it can be identified by an env var.

[Dosant]

If we need, we can avoid introducing a new API altogether, but using saved objects create API. It looks like it allows to import of old objects, as it runs migrations during creation. The con is that we will create new objects.

[jloleysens]

I like this idea a lot! It is definitely helpful to be able to throw a few different saved objects at a migration in a cheap fashion.

I think the current file structure requires at least one more level of namespacing which is currently in the file name (dashboard). I am happy for the namespacing to be in the file name, I just think it should more obviously namespaced e.g., dashboard.lens_by_value.json. I am slightly concerned that the number of files will grow rapidly and a sub-optimal convention might evolve. We might also need to think about running the same migration, but with different mutations of the input since it seems likely this could be the case. Again, this is probably a naming thing, but good to be consistent on.

[Dosant]

Good point! If we get to the implementation, this is definitely something to think through 👍

Maybe this should be a separate test suite all together so this discoverable easier and second level should be "area", so that we could set snapshot code owners

• migrations/
  • dashboard/
    • 7.13.0/
    • 7.12.0/
  • discover/
  • search/

[stacey-gammon]

Thanks for creating this Anton, I'm glad to see some brainstorming!

I think it's a cool idea, and does make writing the snapshot migration tests very easy, but I think there are some limitations with this approach.

It's hard to know what the migrated snapshot test should actually look like, and developers tend to just -u without inspecting. Even in the POC PR, it's hard to look at those before/after files and know if there is a bug. It is even more difficult to know what the snapshot should look like if, for example, the author of a drilldown wanted to write a migration test, they'd have to use a dashboard saved object test, so there would be a lot more content than just the drilldown state.

It also relies on the author of the persistable state to know which saved object to test.

Disclaimer: braindump ahead... it may not make much sense...

I wonder if there is a way to not rely on manual inspection of the snapshot file. I've been thinking lately if we should require every persistable state implementation to implement a data schema validation function, maybe leveraging kbn-schema. If we did, we could perhaps use that instead of manual inspection. However, that would require the saved object author to write one as well, and to make sure that they called their nested persistable state validation schemas. But I'm not sure how the Dashboard Saved Object would validate its panelsJson. I wonder if what gets us into trouble primarily is using JSON.stringify. I wonder if we could have a registry of DataSchemas, and update KbnSchema to validate the nested state. Something like

 DashboardSavedObject.dataSchema = schema.Object({
   title: schema.string(),
   panels: schema.ArrayOfEach(panel => DataSchema.get(panel.type))
 });

And lets say we required SavedObjects to implement PersistableState and required on PersistableState that dataSchema part, we could then do something like

  migrateDashboard(dashboardState: unknown, version: string): typeof DataSchema.get('Dashboard', latestVersion) {
    // This would throw an error if the data was of an unexpected shape.
    const validatedData: typeof DataSchema.get('Dashboard', version) = DataSchema.get('Dashboard', version).validate(dashboardState);
  }

In this DataSchema registry we would have a record of every shape of persistablestate and every version it changed, and a way to run runtime validation on it.

This is reminiscent of the global persistable state registry however, that we talked about awhile ago and decided to not pursue, but maybe because we know more now, we can think about it again.

But the thought is, if we could leverage this data schema validation, instead of requiring a developer to manually inspect a .json snapshot file, I think that would be a big improvement.

---

[Dosant]

On schema validation approach:
In general, I like the idea and I think this something worth considering!
Just a note, that we should avoid running such validations in runtime #97061 #78351, but I assume this could be run only after the migration

I think this also isn't a direct replacement for a snapshot test.
A migration could have a bug, where, let's say it filtered all the panels, so new object has 0 panels: panels: []
The snapshot would show it, but the schema would likely pass.

On snapshot testing::

It's hard to know what the migrated snapshot test should actually look like, and developers tend to just -u without inspecting.

I think your concerns are 100% valid and I think those are also applicable to any kind of snapshot test. Even with the concerns, we use snapshots test, because, I assume, they bring more value than it costs us to maintain them.

Sometimes a developer might ignore the snapshot, sometimes they might notice an unexpected change within the snapshot and will inspect closer.

I think it us important to note, that, this would be just an additional level of testing and not a full replacement to proper functional or other unit tests.

Even in the POC PR, it's hard to look at those before/after files and know if there is a bug

I was thinking if there is a quick way to launch and view the result locally. We could generate a Kibana URL which you could open to preview a resulted saved object and see if works.
But this will get the proposed system more complicated because you'd need to have referenced objects and data in place :(
If we require those, then such tests wouldn't be that simpler to add, than a regular functional test.

Also, I'm thinking... in POC, I am adding a new snapshot, so looking at a new snapshot isn't that useful.
But it would look better if we'd reviewed a change against the existing snapshot.

So maybe, we should try to add these snapshots whenever we introduce a new feature or do the schema change. Then the snapshot in the version when we introduce the object would be the same as the input object.
And only after adding a migration, the snapshot will have to be updated.

It is even more difficult to know what the snapshot should look like if, for example, the author of a drilldown wanted to write a migration test, they'd have to use a dashboard saved object test, so there would be a lot more content than just the drilldown state.
It also relies on the author of the persistable state to know which saved object to test.

I think this is how the flow with such tests could look like:

1. Let's assume I already have a test with a dashboard snapshot and a drilldown in 7.14.
2. I am, a drilldown developer, adding a migration for a drilldown in 7.15
   2.1. I am unit testing it
   2.2 I am functional testing it in the drilldown example plugin
3. I run the CI and see that dashboard snapshot migration with a drilldown from 7.14 has failed.
   3.1. I update the snapshot. Ideally, smoke check drilldowns feature inside the dashboard app and open the PR
4. Dashboard team gets pinged because of the change in their snapshot tests. They see the drilldown is updated. They might smoke check the Dashboard app with drilldowns before giving LGMT.

[joshdover]

I've been thinking about this topic as well as I've been working on adding some documentation around how to test migrations. It's worth nothing that for simple migrations that don't depend on other plugins or registries, this is definitely overkill and unit tests are much more likely to be effective.

For all other migrations, integration tests are a must and the only way to do that properly is to test against a real instance of Kibana. Functional test runner is one way to do this, another would be with the jest_integration suite, which is what I explored in this PR: #106116

One primary advantage to the jest_integration approach is that it requires a simpler set of tools (it's just Jest), however one downside in the current implementation is the development speed. You can't easily start ES and leave it running while you develop, so it will be set up on each suite run. This could be worked around with some sort of development flag that performs this setup conditionally for development only. Though having two separate CLI tools is actually pretty nice since it makes it very explicit that the ES server is still running to avoid any 'dangling process' problems. We'd also need to be sure that however this is turned on and off cannot be checked into the repository unintentionally.

I do agree with @stacey-gammon's concerns around using snapshot testing for this area of coverage. It's critical we get these tests correct and I fear it's too easy to update the snapshot without understanding what or why things changed. OTOH, snapshots make it very easy to see how it changed and track those changes across time. Maybe with the right combination of CODEOWNERS assignments to these snapshots we can be sure that nothing changes without enough people on the team being aware.