-
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 ] One discover security context functional tests #199818
[ Security Solution ] One discover security context functional tests #199818
Conversation
b06148d
to
09d04ec
Compare
/ci |
/ci |
|
||
import { createTestConfig } from '../../config.base'; | ||
|
||
export default createTestConfig({ |
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.
@elastic/kibana-data-discovery team, I have created a separate config for example profiles, because if example profiles are activated, they override any solution profile that may be available.
With this approach we are able to keep tests for example
profiles and solution
profiles separate. Let me know if you think this approach is not optimal.
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 approach makes sense to me and is similar to what O11y did. They kept the existing config and just added their tests to their existing O11y config, but IMO Security can structure it however you'd like as long as the example tests still run.
My only minor nit would be I'd personally go with config.examples.
instead of config.example.
for consistency with other example tests, but just an observation and fine on my end either way.
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.
Done here : 39a02c4
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
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.
Looks good, just some questions.
x-pack/test_serverless/functional/test_suites/security/config.example.context_awareness.ts
Outdated
Show resolved
Hide resolved
...rverless/functional/test_suites/security/ftr/one_discover/context_awareness/cell_renderer.ts
Outdated
Show resolved
Hide resolved
...rverless/functional/test_suites/security/ftr/one_discover/context_awareness/cell_renderer.ts
Outdated
Show resolved
Hide resolved
@@ -9,19 +9,15 @@ import { createTestConfig } from '../../config.base'; | |||
|
|||
export default createTestConfig({ | |||
serverlessProject: 'security', | |||
testFiles: [require.resolve('../common/discover/context_awareness')], | |||
testFiles: [require.resolve('./ftr/one_discover/context_awareness')], |
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.
Curious, what's the one_discover
convention mean semantically?
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.
Actually, One discover
is the name of the project that discover is going through it enables solutions to customize various discover
elements.
I am happy to change it to discover
if one_discover
is confusing and need not be used in code. Will appreciate any comments from you as well @davismcphee
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'd probably recommend going with just discover
instead. We're calling the program One Discover currently, but I imagine these tests could outlive that naming.
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.
Makes sense.. done here: 39a02c4
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.
Left a couple minor comments, but it LGTM! Thanks for adding functional tests 👍
.github/CODEOWNERS
Outdated
/x-pack/test_serverless/functional/test_suites/security/config.examples.ts @elastic/kibana-data-discovery | ||
/x-pack/test_serverless/functional/test_suites/security/config.example.context_awareness.ts @elastic/kibana-data-discovery | ||
/x-pack/test_serverless/functional/test_suites/security/config.context_awareness.ts @elastic/security-threat-hunting-investigations @elastic/kibana-data-discovery | ||
/x-pack/test_serverless/functional/test_suites/security/constants.ts @elastic/kibana-data-discovery |
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.
/x-pack/test_serverless/functional/test_suites/security/constants.ts @elastic/kibana-data-discovery | |
/x-pack/test_serverless/functional/test_suites/security/constants.ts @elastic/security-threat-hunting-investigations |
Should this be assigned to us or Threat Hunting Investigations?
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.
Fixed and changed it to Security Solution
here : 618316c
@@ -9,19 +9,15 @@ import { createTestConfig } from '../../config.base'; | |||
|
|||
export default createTestConfig({ | |||
serverlessProject: 'security', | |||
testFiles: [require.resolve('../common/discover/context_awareness')], | |||
testFiles: [require.resolve('./ftr/one_discover/context_awareness')], |
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'd probably recommend going with just discover
instead. We're calling the program One Discover currently, but I imagine these tests could outlive that naming.
|
||
import { createTestConfig } from '../../config.base'; | ||
|
||
export default createTestConfig({ |
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 approach makes sense to me and is similar to what O11y did. They kept the existing config and just added their tests to their existing O11y config, but IMO Security can structure it however you'd like as long as the example tests still run.
My only minor nit would be I'd personally go with config.examples.
instead of config.example.
for consistency with other example tests, but just an observation and fine on my end either way.
💚 Build Succeeded
Metrics [docs]
History
|
…lastic#199818) ## Summary Fixes elastic/security-team#11112 Follow up to - elastic#199279 Adds functional test for Security Profiles in One Discover. ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: kibanamachine <[email protected]>
Summary
Fixes https://github.com/elastic/security-team/issues/11112
Follow up to
Adds functional test for Security Profiles in One Discover.
Checklist
Delete any items that are not applicable to this PR.