Skip to content

[Cloud Security] POC - basic ui tests for Create Environment workflow#183801

Merged
gurevichdmitry merged 17 commits intoelastic:mainfrom
gurevichdmitry:dg-basic-func-tests-poc
Jun 9, 2024
Merged

[Cloud Security] POC - basic ui tests for Create Environment workflow#183801
gurevichdmitry merged 17 commits intoelastic:mainfrom
gurevichdmitry:dg-basic-func-tests-poc

Conversation

@gurevichdmitry
Copy link
Copy Markdown
Contributor

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.

@gurevichdmitry gurevichdmitry requested a review from a team as a code owner May 19, 2024 14:50
@gurevichdmitry gurevichdmitry added the release_note:skip Skip the PR/issue when compiling release notes label May 19, 2024
@gurevichdmitry gurevichdmitry force-pushed the dg-basic-func-tests-poc branch from da36265 to 1d05710 Compare May 19, 2024 15:08
@gurevichdmitry gurevichdmitry force-pushed the dg-basic-func-tests-poc branch from 1d05710 to 16f7fc6 Compare May 19, 2024 15:35
Copy link
Copy Markdown
Contributor

@kfirpeled kfirpeled left a 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') {
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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') {
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@gurevichdmitry gurevichdmitry requested a review from a team as a code owner May 20, 2024 06:56
@gurevichdmitry gurevichdmitry requested a review from kfirpeled May 20, 2024 07:24
Copy link
Copy Markdown
Contributor

@kfirpeled kfirpeled left a 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

@gurevichdmitry
Copy link
Copy Markdown
Contributor Author

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.

@gurevichdmitry gurevichdmitry requested a review from kfirpeled May 23, 2024 05:53
@gurevichdmitry
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

Copy link
Copy Markdown
Contributor

@opauloh opauloh left a 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);
Copy link
Copy Markdown
Contributor

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);

Copy link
Copy Markdown
Contributor Author

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);
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@gurevichdmitry gurevichdmitry requested a review from opauloh June 2, 2024 11:23
Copy link
Copy Markdown
Contributor

@opauloh opauloh left a 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

@kibana-ci
Copy link
Copy Markdown

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #2 / home app Newsfeed has red icon which is a sign of not checked news

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@gurevichdmitry gurevichdmitry merged commit 252e035 into elastic:main Jun 9, 2024
@kibanamachine kibanamachine added v8.15.0 backport:skip This PR does not require backporting labels Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants