[SIEM] Clean up snapshot #58724
[SIEM] Clean up snapshot #58724
Conversation
|
Pinging @elastic/siem (Team:SIEM) |
angorayc
added
the
release_note:skip
| /> | ||
| </TestProviders> | ||
| ); | ||
| expect(wrapper).toMatchSnapshot(); |
There was a problem hiding this comment.
@angorayc the other snapshot test above does expect(wrapper.find('ZeekDetails')).toMatchSnapshot(). If this test has value, doing the same would prevent us from having to remove the test completely.
There was a problem hiding this comment.
Thanks @rylnd, that's a good idea. Actually I added that line for manual debugging in my previous PR and forgot to remove that.
e97b451#r37522518
@FrankHassanabad would you want me to move this snapshot test to another block and apply expect(wrapper.find('ZeekDetails')).toMatchSnapshot() or it is ok if I just remove this check?
There was a problem hiding this comment.
I think the team consensus has been that snapshots are optional but no one really cares if we delete them if there are other tests that provide coverage.
So, yeah, you are fine to delete this snapshot test if you want to. They don't provide us with a lot of value compared to focused testing.
But if you find value in them and want to continue using them (as long as they are not huge outputs) that is fine as well.
So, in short, optionally you can delete any ones you want to or keep using them as long as they are not large snapshots.
There was a problem hiding this comment.
ok, given that I added it accidentally, removing it here. Thank you @rylnd @FrankHassanabad
|
jenkins, test this |
|
jenkins, test this |
💚 Build SucceededTo update your PR or re-run it, just comment with: |
6 participants
Summary
#58723
Checklist
Delete any items that are not applicable to this PR.
[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] Unit or functional tests were updated or added to match the most common scenarios[ ] This was checked for keyboard-only and screenreader accessibility[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser[ ] This was checked for cross-browser compatibility, including a check against IE11For maintainers
[ ] This was checked for breaking API changes and was labeled appropriately