Skip to content
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

Merged
merged 17 commits into from
Jun 9, 2024

Conversation

gurevichdmitry
Copy link
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
Copy link
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
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
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
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
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
Copy link
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

.buildkite/ftr_configs.yml Show resolved Hide resolved
@gurevichdmitry
Copy link
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
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
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
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
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
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
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
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
Collaborator

💛 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
18 checks passed
@kibanamachine kibanamachine added v8.15.0 backport:skip This commit 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 commit 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.

None yet

6 participants