-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Security Solution] Cypress back to live #86093
Conversation
@elasticmachine merge upstream |
@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.
Added some cursory comments. Going to run this locally a bit and then circle back for approval.
|
||
const ABSOLUTE_DATE = { | ||
endTime: '2019-08-01T20:33:29.186Z', | ||
startTime: '2019-08-01T20:03:29.186Z', | ||
}; | ||
|
||
describe('URL compatibility', () => { | ||
before(() => { |
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.
How are we deciding between before
and beforeEach
in these tests? Unless there's a huge performance cost my preference is always for beforeEach
to minimize shared state between tests.
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 understand your point, but there are specific scenarios were there is not going to be shared state of the tests that can impact the behaviour and is going to save us execution time.
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.
What are the scenarios you're referring to, here? The use of cleanKibana
in before
only works if each test is allowed to complete its teardown:
- If a test fails after it's created a rule and
deleteRule
is part of the test itself, then the rule SO will be persisted to the next test - If a test fails before it's created a rule and
deleteRule
is part of anafter*
hook,deleteRule
will fail and cause cypress to skip the remaining tests
I personally would rather have a slow test than a flaky one. Performance and reliability are (with the exception of timeouts) mostly independent issues, and reliability is the focus at the moment. IMO we've also got many more options for improving performance/speed of these tests external to the tests themselves (more CI groups, beefier CI machines, etc.) such that I don't think we need to be sacrificing reliability for performance right now.
@@ -39,6 +46,7 @@ describe('value lists', () => { | |||
}); | |||
|
|||
afterEach(() => { | |||
removeSignalsIndex(); |
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.
If all our tests use cleanKibana
in a beforeEach
, I don't think we need the same in afterEach
.
However, that does bring up a general question about teardown: what is the cypress suite's teardown responsibility as a whole? Is cypress relegated to its own CI group and so we don't have to worry about cleaning up for the "next" suite? Is there a possibility that tests will be added to the cypress group and we do need to start caring about this?
I'm thinking about the situations that have bit us in the past, namely incomplete teardown and shared state, which are strongly related. We should do whatever we can to minimize those and isolate tests.
@@ -21,16 +21,13 @@ | |||
|
|||
// Import commands.js using ES2015 syntax: | |||
import './commands'; | |||
import 'cypress-promise/register'; |
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.
Why was this removed? Can we remove the package as well? It seems unused now.
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.
Because we are not using it. I didn't remove the package since now we are using a shared package.json and we are not the only ones using cypress.
return false; | ||
} | ||
Cypress.on('uncaught:exception', () => { | ||
return false; |
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 means that we ignore any uncaught applications, right? This seems like a big change unless I'm misunderstanding. Were we seeing false negatives?
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.
Without that line, cypress fails each time that an error is raised in the console log. Do we want this behaviour to be caught? Because right now there are some tests failing because of that although the UI looks fine.
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.
Got it. It would be nice to log the error somewhere rather than discard it, but that might be too noisy for CI logs 🤷♂️ ? I defer to you.
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 think some wires got crossed on the afterEach thing; I think those should (need to?) go back into after*
hooks. Sorry about that!
Also see my idea about meta-programming most of the test setup/teardown for specific use cases; I think that could clean up a lot of complexity here.
'have.text', | ||
expectedNumberOfOpenedAlerts.toString() | ||
); | ||
esArchiverUnload('alerts'); |
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 think these should go back into the afterEach
. I had mentioned offline that, due to cypress skipping the suite if the test fails, we should move any test-specific teardown (e.g. deleteRule
) into the test itself to avoid unnecessary skips, but I don't think that these unload
s fall in that category.
@@ -34,16 +35,13 @@ import { DETECTIONS_URL } from '../urls/navigation'; | |||
describe('Alerts', () => { | |||
context('Closing alerts', () => { | |||
beforeEach(() => { | |||
cleanKibana(); | |||
removeSignalsIndex(); |
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 is a bit of a tangent, but I just had a couple thoughts:
- Keeping test setup/teardown symmetrical is a PITA
- reorienting this to do teardown, then setup all within setup would reduce this complexity
- This would address/sidestep the issue of test failures, which can currently cause teardown not to execute
- We could lessen developer burden by defining this as a general function(s)
- reorienting this to do teardown, then setup all within setup would reduce this complexity
// in test file
describe('my test', () => {
detectionEngineHooks();
it('does a thing', () => {});
});
// in helper file
const detectionEngineHooks = () => {
beforeEach(() => {
cleanKibana(); // deletes `.kibana*`, loads `empty_kibana`
cleanSignals(); // deletes `.siem-signals*`, hits detections endpoint to create index
cleanLoad('alerts'); // see below
});
};
const cleanLoad = (archive) => {
unload(archive); // might need to try/catch this?
load(archive);
}
💚 Build SucceededMetrics [docs]Distributable file count
History
To update your PR or re-run it, just comment with: |
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.
Thank you for taking the time to dig into these! We've got some fun ideas to play with in the near future here, but let's merge this and keep things rolling. Upwards and onwards!
* removes signals extra sanity * fixes signals cleaning * cleans kibana before each test execution * upgrades cypress to version 6.1.0 * enables cypress execution on jenkins * generalises kibana cleanining indexes * cleans after hooks * fixes type check errors * moves archive unloads to after hooks * fixes alert test * skips failed tests Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* removes signals extra sanity * fixes signals cleaning * cleans kibana before each test execution * upgrades cypress to version 6.1.0 * enables cypress execution on jenkins * generalises kibana cleanining indexes * cleans after hooks * fixes type check errors * moves archive unloads to after hooks * fixes alert test * skips failed tests Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* removes signals extra sanity * fixes signals cleaning * cleans kibana before each test execution * upgrades cypress to version 6.1.0 * enables cypress execution on jenkins * generalises kibana cleanining indexes * cleans after hooks * fixes type check errors * moves archive unloads to after hooks * fixes alert test * skips failed tests Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* removes signals extra sanity * fixes signals cleaning * cleans kibana before each test execution * upgrades cypress to version 6.1.0 * enables cypress execution on jenkins * generalises kibana cleanining indexes * cleans after hooks * fixes type check errors * moves archive unloads to after hooks * fixes alert test * skips failed tests Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
In this PR we are bringing back Cypress tests to live.
In a different branch I'm working in order to minimise even more the use of es_archive.
@rylnd lots of thanks for making this PR possible ❤️