-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lens on Dashboard 7.12.1 Smoke Tests #102667
Lens on Dashboard 7.12.1 Smoke Tests #102667
Conversation
beda769
to
49c5ddd
Compare
49c5ddd
to
d80239b
Compare
Pinging @elastic/kibana-presentation (Team:Presentation) |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @ThomThomson ! Thanks for adding this test coverage. I left a couple comments that I'd like to get your thoughts on.
}); | ||
|
||
it('should be able to import dashboard with various Lens panels from 7.12.1', async () => { | ||
await PageObjects.savedObjects.importFile( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spoke with @tylersmalley about how we might load legacy saved object data into Kibana and he mentioned we should use the kbnArchiver
which is a tool that aims to replace esArchiver
and other methods for loading of saved objects. Example usage:
await kibanaServer.importExport.load( |
This will also interact with Kibana via the API rather than the UI to load data which should be faster. Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will look into this! Anything that speeds up the run is a good thing. I assume that this can support ndjson
files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so! The kbn_archiver.js script also allows for extracting those values from the cluster. For example:
node scripts/kbn_archiver.js --config test/functional/config.js save test/functional/fixtures/kbn_archiver/foo.json --type index-pattern,dashboard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In speaking to @tylersmalley, it like it doesn't support ndjson
, and I would need to load up 7.12 again and re-export. It's probably worth mentioning this in the description so that people know it's an option going forward, but I don't think it's necessary to update to this format on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have an open issue to add support for a pretty format to the saved object export which would allow us to directly support that format: #103021
await esArchiver.load('x-pack/test/functional/es_archives/empty_kibana'); | ||
}); | ||
|
||
it('should be able to import dashboard with various Lens panels from 7.12.1', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not happen as part of the before
setup? I might be missing something though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be a part of the before
setup because it is required by all subsequent tests - I moved it into its own test because the test fails if a migration error occurs, and I wanted the failed test name to be more explicit. I will take a look into moving most of this into before
but leaving the import success validation as its own test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding these tests @ThomThomson ! I will approve to unblock your progress 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
* added smoke tests for lens by value panels on dashboard
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Summary
part of #102416
This PR adds dashboard migration smoke tests for a variety of lens panels. These will ensure that every time a migration issue is introduced that would break Lens panels on dashboards, it is caught by the CI.
These smoke tests are written in such a way that they would have caught #101430 and #100756 before the 7.13 release.
It works by importing a dashboard last saved in 7.12.1 which contain 6 lens panels in different configurations, with and without drilldowns, by value and by reference. It then checks that all panels load correctly, that there are no error embeddables, that each panel can be edited when in edit mode, and that all drilldowns were retained.
How to test
Preventing severe errors during migration:
You can simulate #101430 by adding the following if statement after line
29
insrc/plugins/embeddable/common/lib/migrate.ts
With this simulated error, the smoke test should fail when importing the dashboard saved object, as by value embeddables with drilldowns were included in the smoke test dashboard.
Preventing severe runtime errors after migration:
You can simulate #100756 by adding the following if statement after line
1210
inx-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts
With this simulated error, the lens panels with the Average operation will not render, and the smoke test will stall waiting for the dashboard to render. This will fail the test.
Other error cases covered
If a Lens panel is able to render, but renders as an
ErrorEmbeddable
the smoke test will fail.If a Lens panel is able to correctly render, but does not have an edit action when the dashboard is in edit mode, the test will fail. This is often a sign of something else going wrong.
Going forward
Short Term
Short term, more of this style of smoke test will be written, covering Visualize and Maps migrations from 7.12.1. This way all combinations of by value, by reference, and enhanced panels will have test coverage going forward. The process for creating these smoke tests will be as follows:
esArchiver
to load in a dataset that can be easily used in functional tests, for instance:node scripts/es_archiver load getting_started/shakespeare --config test/functional/config.js --es-url=http://localhost:9200 --kibana-url=http://localhost:5601
ndjson
export to thex-pack/test/functional/apps/dashboard/migration_smoke_tests/exports
folderLong Term
In the past, we have had a process where when creating new features, functional tests which cover those features usually create new saved objects during the course of the test, using whatever the latest version happens to be.
This process needs to change for the long term, to promote the creation of additional migration smoke tests like this one for all new features which have some component of state persistence. A sample flow for this kind of process would be:
.ndjson
file.ndjson
in the repo. This effectively freezes the state to be exactly as it was when created.Ideally, we could get to a state where each new feature that persists state runs exactly the same functional test suite on state which has been migrated from the version in which it was initially created, and on state that is newly created in the current version.
PR Items
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers