-
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
Add model version testing section to documentation #168453
Add model version testing section to documentation #168453
Conversation
Pinging @elastic/kibana-core (Team:Core) |
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! Just 1 nit
describe('Model version 2', () => { | ||
it('properly backfill the expected fields when converting from v1 to v2', () => { | ||
const obj = createSomeSavedObject(); | ||
|
||
const migrated = migrator.migrate({ | ||
document: obj, | ||
fromVersion: 1, | ||
toVersion: 2, | ||
}); | ||
|
||
expect(migrated.properties).toEqual(myExpectedProperties); | ||
}); | ||
|
||
it('properly removes the expected fields when converting from v2 to v1', () => { | ||
const obj = createSomeSavedObject(); | ||
|
||
const migrated = migrator.migrate({ | ||
document: obj, | ||
fromVersion: 2, | ||
toVersion: 1, | ||
}); | ||
|
||
expect(migrated.properties).toEqual(myExpectedProperties); | ||
}); | ||
}); |
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.
For best practices, should we recommend having a global myExpectedProperties
for each version so that we can test symmetric migrations (when possible)
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'd say it's probably a developer's preference? e.g I usually use different documents / attributes between different tests (just to increase the potential branch coverage), so I think I would be using different attributes when testing the up and down parts of the migration 😄
## Summary Follow-up of elastic#167501 and elastic#167861 Add section in the model version documentation about testing plan
Summary
Follow-up of #167501 and #167861
Add section in the model version documentation about testing plan