-
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
[ML] Add tests for anomaly embeddables migrations #116520
[ML] Add tests for anomaly embeddables migrations #116520
Conversation
@elasticmachine merge upstream |
@elasticmachine merge upstream |
Pinging @elastic/ml-ui (:ml) |
|
||
describe('supports migrations', function () { | ||
const panelTitle = `Saved ML anomaly charts for fq_multi_1_ae`; | ||
const dashboardSavedObject = { |
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.
Probably worth moving the saved object configs and then looping through each item in the list, making sure it can be loaded successfully. Then for 8.0, a 7.16
config can be added with minimal changes to the code required.
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.
Updated here eee3561
await PageObjects.common.navigateToApp('dashboard'); | ||
}); | ||
|
||
it('loads saved dashboard from version 7.15', 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.
Could the coreMigrationVersion
from the saved object config be used in the test title?
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.
Updated here eee3561
x-pack/test/functional/apps/ml/embeddables/anomaly_embeddables_migration.ts
Outdated
Show resolved
Hide resolved
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.
Tested locally and overall looks good. Agree with @pheyos that the step that sets the time range is unnecessary, but other than that LGTM. Just left one extra comment about a change to the test title.
|
||
for (const testData of testDataList) { | ||
const { dashboardSavedObject, panelTitle, type } = testData; | ||
describe(`loads saved dashboard from version ${dashboardSavedObject.coreMigrationVersion}`, function () { |
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.
This same description is used for the test below. I think it would be good to include the panelTitle
in the description here too.
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.
Agreed - please let's not repeat this long title and have a shorter one for either the suite or the 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.
Updated here 7e51b01
4dbb311
to
7e51b01
Compare
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
@elasticmachine merge upstream |
Started flaky test suite runner ... successful in 40 runs ✅ |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @qn895 |
The following labels were identified as gaps in your version labels and will be added automatically:
If any of these should not be on your pull request, please manually remove them. |
* [ML] Add tests for anomaly charts embeddable migrations * [ML] Broaden tests for anomaly swimlane as well * [ML] Fix function rename * [ML] Update tests to use bulk api * [ML] Remove override Co-authored-by: Kibana Machine <[email protected]>
* [ML] Add tests for anomaly charts embeddable migrations * [ML] Broaden tests for anomaly swimlane as well * [ML] Fix function rename * [ML] Update tests to use bulk api * [ML] Remove override Co-authored-by: Kibana Machine <[email protected]>
* [ML] Add tests for anomaly charts embeddable migrations * [ML] Broaden tests for anomaly swimlane as well * [ML] Fix function rename * [ML] Update tests to use bulk api * [ML] Remove override Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Quynh Nguyen <[email protected]>
Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync. |
* [ML] Add tests for anomaly charts embeddable migrations * [ML] Broaden tests for anomaly swimlane as well * [ML] Fix function rename * [ML] Update tests to use bulk api * [ML] Remove override Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Quynh Nguyen <[email protected]>
Summary
Part of #110949. This PR adds functional tests for anomaly swimlane and anomaly charts embeddable which loads a 7.15 dashboard JSON object into 8.0.0
Checklist
Delete any items that are not applicable to this PR.