-
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 5 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.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,7 +147,7 @@ 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 () => { | ||
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 +187,7 @@ export default ({ getService }: FtrProviderContext) => { | |
expect(body?.execution_summary?.last_execution?.status).toBe('succeeded'); | ||
}); | ||
|
||
it('@skipInQA adds a webhook to an immutable rule', async () => { | ||
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 +210,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 +224,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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,7 @@ export default ({ getService }: FtrProviderContext): void => { | |
/* from a package that was bundled with Kibana */ | ||
// | ||
// FLAKY: https://github.com/elastic/kibana/issues/180087 | ||
describe.skip('@ess @serverless @skipInQA install_bundled_prebuilt_rules', () => { | ||
describe.skip('@ess @serverless @skipInServerless install_bundled_prebuilt_rules', () => { | ||
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. @MadameSheema We don't want all the tests for prebuilt rules to be skipped in all quality gates. We need them to run on CI, where, as far as I'm aware, they are not currently skipped and have been passing. This also applies to some of the other rule management tests that are being skipped in CI by this PR. I would suggest to:
I'd personally choose the 2nd option as being more explicit about where a test should run. Sorry if I missed this in #179737 where Cypress tests for prebuilt rules and rule management have been skipped in Serverless CI. We'd need to run those Cypress tests in CI too, so I'd propose the same changes for Cypress tests. 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. @maximpn @yctercero since you were the ones against having extra labels, can you please arrive to an agreement about how to proceed with it? This is my proposal:
If you don’t want a test to be executed as part of the second quality gate you don’t add the 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. @MadameSheema do I get it correctly a test which should be skipped in MKI env but run everywhere else will have the following tags describe('@ess @serverless @serverlessQA @skipServerlessMKI install_bundled_prebuilt_rules', () => { Here we return to the situation when it equals to describe('@ess @serverless install_bundled_prebuilt_rules', () => { Here we have two options
The second approach looks better for automatic skipping script. When a test fails in some env or becomes flaky it could be skipped automatically. I checked the PR's diff once more time and see that it's more convenient to use the second approach. Quite often 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 set of tags doesn't allow to run a test on CI in ESS + on CI in Serverless, but not run it in the periodic pipeline + release gates. At this point, we need to be able to run many of our tests ONLY on CI in ESS + on CI in Serverless, because they are broken in MKI environments. Fixing this requires time and effort we don't have at the moment.
This set of tags also doesn't allow to run a test on CI in ESS + on CI in Serverless, but not run it in the periodic pipeline + release gates. We don't need to be able to skip a test on CI in Serverless, but run it in MKI pipelines. We don't have such class of tests as of now. 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. Could you clarify the requirements to running tests in periodic pipeline? Do we need a separate way to skip tests for periodic pipeline? In my understanding if a test is broken in MKI env it will be broken in periodic pipeline since it's an MKI env as well. Taking this into account a test shouldn't have 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 there are differences between the serverless environment we use on CI and a real one but at the same time, we don't want to execute all the tests in the quality gate, is a way for us to determine the health of our application in a real serverless project environment. We need a separate way of skipping tests from MKI but not CI. A test with just
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. @MadameSheema Your proposal works for me 👍 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. Above proposal looks great to me @MadameSheema 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. Me and @banderror discussed the approach on a zoom call. Tags you suggest in the last comment look good to me. I have some comments though ESS tags are pretty clear. We need to run tests against ESS env and skip broken tests.
Serverless in general is pretty clear as well. We need to run test against Serverless env (abstractly it doesn't matter which one) and skip broken tests
If we don't need to run tests against some env an appropriate tag The most interesting part start from the fact that Serverless envs are different. Not all tests work in Serverless MKI env currently. Taking into account this fact we need an ability to skip such tests so we can use the following tag
And the last part is that we need some test to block Serverless release when critical functionality doesn't work. In this case something explicit like
I have a question about your suggestion. Why do you want to use explicit cc @rylnd 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. Maybe at this point it's best to jump on a Zoom. I'm a bit confused as to what's being asked here anymore. @MadameSheema worked on this PR after we chatted and decided it'd be great for our API and cypress tags to match so this PR brought them into parity. We had discussed and decided on the tags during the first PR that changed the Cypress tags. It would be great to finalize the discussion so we don't keep rehashing the discussion in each PR. |
||
beforeEach(async () => { | ||
await deleteAllRules(supertest, log); | ||
await deleteAllPrebuiltRuleAssets(es, log); | ||
|
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.
Should we add all tags to this list?