-
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
[Cloud Security] POC - basic ui tests for Create Environment workflow #183801
[Cloud Security] POC - basic ui tests for Create Environment workflow #183801
Conversation
da36265
to
1d05710
Compare
1d05710
to
16f7fc6
Compare
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 ok, but please refactor the settings into new files
}, | ||
}; | ||
// If TEST_CLOUD environment varible is defined, return the configuration for cloud testing | ||
if (process.env.TEST_CLOUD !== '1') { |
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.
instead of if statement please refactor the following into a new config file: config.cloud.ts
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
loadTestFile(require.resolve('./findings_old_data')); | ||
loadTestFile(require.resolve('./vulnerabilities')); | ||
loadTestFile(require.resolve('./vulnerabilities_grouping')); | ||
if (process.env.TEST_CLOUD === '1') { |
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.
instead of an if statement please refactor the following into a new file
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
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.
Besides removing it from running on every pull request, do you mind to enhance our documentation and share how to run these sanity tests?
Other then that, it looks pretty solid to me
Added a README file describing how to configure and execute tests, both for development purposes and on demand. |
@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.
It looks good, just got some questions regarding the assertions
|
||
it('displays correct number of resources evaluated', async () => { | ||
const resourcesEvaluated = await dashboard.getKubernetesResourcesEvaluated(); | ||
expect((await resourcesEvaluated.getVisibleText()) === '199').to.be(true); |
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.
So it's ensured these numbers won't change over time? otherwise, I would recommend changing to something like the previous test leaving some room for fluctuation:
expect(resourcesEvaluatedCount).greaterThan(150);
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.
@opauloh, since we are deploying the same resources, the number should not change. However, the timing of findings retrieval can affect this. Therefore, I have taken your suggestion and updated the test to allow some room for fluctuation
it('displays accurate summary compliance score', async () => { | ||
await pageObjects.header.waitUntilLoadingHasFinished(); | ||
const scoreElement = await dashboard.getKubernetesComplianceScore(); | ||
expect((await scoreElement.getVisibleText()) === '83%').to.be(true); |
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.
More like the previous question, will the agents be installed pointing to a controlled environment that the score doesn't (or at least shouldn't) change over time? otherwise, we could perform an assertion that are more flexible:
expect(score).greaterThan(80);
The main reason is having to avoid having to update the tests in Kibana whenever these values change if the findings of the environments scanned are mutable
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
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.
LGTM - Great starting point
💛 Build succeeded, but was flaky
Failed CI StepsTest FailuresMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Summary
This PR is essential for a Proof of Concept (POC) aimed at testing the feasibility of running UI tests written in the Kibana repository within a separate repository using GitHub Actions.