[Security Solution][Detection Engine] Getting rid off before hook if is not loading an archive #178876
Conversation
|
/ci |
|
/ci |
MadameSheema
added
release_note:skip
|
Pinging @elastic/security-detection-engine (Team:Detection Engine) |
|
/ci |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]
History
To update your PR or re-run it, just comment with: |
| }); | ||
|
|
||
| after(() => { | ||
| cy.task('esArchiverUnload', { archiveName: 'exceptions' }); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| deleteDataView(DATAVIEW); |
There was a problem hiding this comment.
It looks like DATAVIEW will still exist after the last test is run. Should there be an additional afterEach with a deleteDataView(DATAVIEW)?
| before(() => { | ||
| beforeEach(() => { | ||
| deleteAlertsAndRules(); | ||
| deleteExceptionLists(); |
There was a problem hiding this comment.
Seeing the same pattern as above here: after the last test, any rules and exception lists created will still be present for the next test. There should probably also be an afterEach added with these commands.
Trying to set up a pristine state in the beforeEach is a good, defensive practice, but cleaning up your own test's data is much more important. I suspect that's been the cause of some flakiness in our suite.
| deleteRuntimeField('security-solution-default', alertRunTimeField); | ||
| deleteRuntimeField('security-solution-default', timelineRuntimeField); | ||
| }); | ||
|
|
||
| beforeEach(() => { |
There was a problem hiding this comment.
Same here, no afterEach for cleanup :(
| @@ -16,7 +16,7 @@ import { login } from '../../../../tasks/login'; | |||
| const dataViews = ['auditbeat-*,fakebeat-*', 'auditbeat-*,*beat*,siem-read*,.kibana*,fakebeat-*']; | |||
|
|
|||
| describe('Sourcerer permissions', { tags: ['@ess', '@brokenInServerless'] }, () => { | |||
| before(() => { | |||
| beforeEach(() => { | |||
Hey Ryland!! The above changes have been introduced in order to make sure the Cypress retriability works as expected. It is true that the test will not leave a clean environment but, the execution of it will create the clean environment. That was done like that so in case of failure when you are developing or debugging, the state persists and you can check what really happened checking the environment. If you feel more comfortable with the afterEach, I've no problem about moving the beforeEach changes to an afterEach. Please, let me know. |
The more typical solution for persisting state when debugging/developing is to simply comment out the "cleanup" steps (be they in Leaving state for the next test (do our suites run in a fixed order, or randomly?) is just asking for trouble, in my experience. I'm not saying that it's the cause of any of our current flakiness, but it's certainly not making things any easier, and I don't think it should be the default approach. However, these are certainly not the only tests to follow this pattern, so we should create a broader issue for this, discuss, and decide if/how to address. cc @marshallmain @yctercero |
@rylnd would you mind creating the issue for us to discuss? It would be great to track it and I agree that it is worth discussing and addressing. We do have a large amount of flake tickets open right now, and it is worth examining if addressing this issue could help. |
Summary
Addresses: #175021
Before the introduced changes on this PR, we had test dependency at
manage_lists_cy.ts. Now that the dependency does not exist anymore, we had to update the assertions.Update:
Flaky test suite runner passed successfully