-
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
[Security Solution] Adds serverlessQA tag to the API tests #180773
Changes from 11 commits
973d5da
ec2c07b
77fa125
08b8ebc
b052b7f
dae1ce9
2137e9e
3980615
39270f7
0cc8b13
37d3b3a
cfdaefb
dc14e41
6ff7457
78760a6
1a6dc9a
7dae577
3e1dd97
0ac31e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,16 +15,25 @@ we have introduced the `detection_response` directory to consolidate all the int | |
|
||
- In this directory, Mocha tagging is utilized to assign tags to specific test suites and individual test cases. This tagging system enables the ability to selectively apply tags to test suites and test cases, facilitating the exclusion of specific test cases within a test suite as needed. | ||
|
||
- There are three primary tags that have been defined: @ess, @serverless, and @brokenInServerless | ||
|
||
- Test suites and cases are prefixed with specific tags to determine their execution in particular environments or to exclude them from specific environments. | ||
|
||
- We are using the following tags: | ||
* `@ess`: Runs in an ESS environment (on-prem installation) as part of the CI validation on PRs. | ||
|
||
* `@serverless`: Runs in the first quality gate and in the periodic pipeline. | ||
|
||
* `@serverlessQA`: Runs in the second quality gate. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This tag seems to be strongly tied to the concept of a "release" (as evidenced in the command names, and this PR's description), but that isn't really reflected in either the README here nor in the tag name itself. Would There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As part of the release, the tests might be executed as part of 2 other quality gates and we might have some on the second one that we don't want to execute it on the third one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO referencing the "gate order" might be the most straightforward way to express this; it's not intuitive that |
||
|
||
* `@skipInEss`: Skipped for ESS environment. | ||
|
||
* `@skipInServerless`: Skipped for all quality gates and periodic pipeline. | ||
|
||
ex: | ||
``` | ||
describe('@serverless @ess create_rules', () => { ==> tests in this suite will run in both Ess and Serverless | ||
describe('creating rules', () => {}); | ||
|
||
describe('@brokenInServerless missing timestamps', () => {}); ==> tests in this suite will be excluded in Serverless | ||
describe('@skipInServerless missing timestamps', () => {}); ==> tests in this suite will be excluded in Serverless | ||
rylnd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
``` | ||
|
||
|
@@ -86,7 +95,7 @@ In this project, you can run various commands to execute tests and workflows, ea | |
``` | ||
3. **Run tests for "exception_workflows" using the serverless runner in the "qaEnv" environment:** | ||
```shell | ||
npm run run-tests:dr:default exceptions/workflows serverless qaEnv | ||
npm run run-tests:dr:default exceptions/workflows serverless qaPeriodicEnv | ||
``` | ||
4. **Run the server for "exception_workflows" in the "essEnv" environment:** | ||
```shell | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,15 +21,19 @@ let grepArgs = []; | |
if (type !== 'server') { | ||
switch (environment) { | ||
case 'serverlessEnv': | ||
grepArgs = ['--grep', '/^(?!.*@brokenInServerless).*@serverless.*/']; | ||
grepArgs = ['--grep', '/^(?!.*@skipInServerless).*@serverless.*/']; | ||
break; | ||
|
||
case 'essEnv': | ||
grepArgs = ['--grep', '/^(?!.*@brokenInEss).*@ess.*/']; | ||
grepArgs = ['--grep', '/^(?!.*@skipInEss).*@ess.*/']; | ||
break; | ||
|
||
case 'qaPeriodicEnv': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do I get it right all Serverless tests besides skipped ones will run periodically against MKI env? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly :) |
||
grepArgs = ['--grep', '/^(?!.*@skipInServerless).*@serverless.*/']; | ||
break; | ||
|
||
case 'qaEnv': | ||
grepArgs = ['--grep', '/^(?!.*@brokenInServerless|.*@skipInQA).*@serverless.*/']; | ||
grepArgs = ['--grep', '/^(?!.*@skipInServerless).*@serverlessQA.*/']; | ||
break; | ||
|
||
default: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,7 +147,8 @@ export default ({ getService }: FtrProviderContext) => { | |
expect(body?.execution_summary?.last_execution?.status).toBe('succeeded'); | ||
}); | ||
|
||
it('@skipInQA expects an updated rule with a webhook action and meta field runs successfully', async () => { | ||
// Broken in MKI environment, needs triage | ||
it('@skipInServerless expects an updated rule with a webhook action and meta field runs successfully', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a comment here:
Just so we know it needs follow up. |
||
const webhookAction = await createWebHookRuleAction(supertest); | ||
|
||
await supertest | ||
|
@@ -187,7 +188,8 @@ export default ({ getService }: FtrProviderContext) => { | |
expect(body?.execution_summary?.last_execution?.status).toBe('succeeded'); | ||
}); | ||
|
||
it('@skipInQA adds a webhook to an immutable rule', async () => { | ||
// Broken in MKI environment, needs triage | ||
it('@skipInServerless adds a webhook to an immutable rule', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a comment here:
Just so we know it needs follow up. |
||
const immutableRule = await getImmutableRule(); | ||
const webhookAction = await createWebHookRuleAction(supertest); | ||
const ruleAction = { | ||
|
@@ -210,7 +212,7 @@ export default ({ getService }: FtrProviderContext) => { | |
expect(updatedRule.throttle).toEqual(immutableRule.throttle); | ||
}); | ||
|
||
it('@skipInQA should be able to create a new webhook action, attach it to an immutable rule and the count of prepackaged rules should not increase. If this fails, suspect the immutable tags are not staying on the rule correctly.', async () => { | ||
it('@skipInServerless should be able to create a new webhook action, attach it to an immutable rule and the count of prepackaged rules should not increase. If this fails, suspect the immutable tags are not staying on the rule correctly.', async () => { | ||
const immutableRule = await getImmutableRule(); | ||
const hookAction = await createWebHookRuleAction(supertest); | ||
const ruleToUpdate = getRuleWithWebHookAction( | ||
|
@@ -224,7 +226,7 @@ export default ({ getService }: FtrProviderContext) => { | |
expect(status.rules_not_installed).toBe(0); | ||
}); | ||
|
||
it('@skipInQA should be able to create a new webhook action, attach it to an immutable rule and the rule should stay immutable when searching against immutable tags', async () => { | ||
it('@skipInServerless should be able to create a new webhook action, attach it to an immutable rule and the rule should stay immutable when searching against immutable tags', async () => { | ||
const immutableRule = await getImmutableRule(); | ||
const webhookAction = await createWebHookRuleAction(supertest); | ||
const ruleAction = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,8 +40,8 @@ export default ({ getService }: FtrProviderContext) => { | |
const dataPathBuilder = new EsArchivePathBuilder(isServerless); | ||
const path = dataPathBuilder.getPath('auditbeat/hosts'); | ||
|
||
// Intentionally setting as @skipInQA, keeping tests running in MKI that should block release | ||
describe('@ess @serverless @skipInQA set_alert_tags', () => { | ||
// Intentionally setting as @skipInServerless, keeping tests running in MKI that should block release | ||
describe('@ess @serverless @skipInServerless set_alert_tags', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one's a bit tricky - we technically need to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Per the agreement we did on the Cypress PR: #179737 this is not possible without having an extra tag. Currently for both API and cypress:
|
||
describe('validation checks', () => { | ||
it('should give errors when no alert ids are provided', async () => { | ||
const { body } = await supertest | ||
|
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.
The only other mention of "first quality gate" is in the cypress README; I think it's worth defining these terms in some detail since they're not going to be self-evident to everyone.
Same thing for "periodic pipeline," "second quality gate," and "all quality gates."
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.
Hey @rylnd everything is already documented.
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.
@MadameSheema it's wonderful that it's documented, but how does someone reading this README find those definitions? You shared links to quality gates and periodic pipeline with me individually, but that process does not scale, so I'm proposing that we somehow reference that documentation here.
If we are reluctant/unable to share those docs because they're internal, I think that might be indication that this README either covers too much or is in the wrong place; our public documentation shouldn't reference internal processes that the reader has no insight on.
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.
++ to @rylnd's comment
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.
Yup, share it with you in private due to being internal documentation, tbh I don't know what is best, thoughts? What do you prefer?
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 the ideal approach in my mind would be:
I'm imagining a contributor who's written a cypress test for a feature they'd like to add; I would not expect them to know/care about any of these tags. As part of the review process, an Elastician would tell them which tags need to be added to their tests.
Generally: if we can't provide the reader with enough information to determine whether they should use the tag, I don't think that it's helpful to mention it at all.
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.
Maybe a passing mention of the tags as "related to internal release processes" would be a way to both define the tags but also deemphasize them, here?