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

[Observability Onboarding] Migrate e2e Playwright tests from oblt-playwright repo #203616

Merged

Conversation

mykolaharmash
Copy link
Contributor

@mykolaharmash mykolaharmash commented Dec 10, 2024

Closes #199016

This change migrates and and expands tests from oblt-playwright repo.

These tests are part of the Nightly workflow and being run by an Ensemble story on the CI.

The Nightly workflow itself is still in development and does not support some of the use cases, that's why kubernetes tests are skipped for now in this PR.

See the ./README.md on how to run the tests locally.

@mykolaharmash mykolaharmash requested a review from a team as a code owner December 10, 2024 15:31
@mykolaharmash mykolaharmash requested a review from a team December 10, 2024 15:31
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Dec 10, 2024
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@mykolaharmash mykolaharmash added backport:skip This commit does not require backporting Team:obs-ux-logs Observability Logs User Experience Team labels Dec 10, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

@mykolaharmash mykolaharmash added the release_note:skip Skip the PR/issue when compiling release notes label Dec 10, 2024
@flash1293
Copy link
Contributor

flash1293 commented Dec 11, 2024

Hey, thanks for this! I tested both auto-detect and k8s flows and the while the auto-detect one was working fine, the kubernetes one opened the table flyout for the nodes visualization to early and it was still empty. Refreshing the page and opening the flyout manually made the test pass, I guess we need a similar retry loop here like the one you added for auto-detect and the host overview page?

Other than that LGTM

@mykolaharmash mykolaharmash force-pushed the 199016-moving-tests-auto-detect-only branch from 2a97c3c to 3c5ab27 Compare December 11, 2024 14:17
@mykolaharmash
Copy link
Contributor Author

Hey, thanks for this! I tested both auto-detect and k8s flows and the while the auto-detect one was working fine, the kubernetes one opened the table flyout for the nodes visualization to early and it was still empty. Refreshing the page and opening the flyout manually made the test pass, I guess we need a similar retry loop here like the one you added for auto-detect and the host overview page?

Other than that LGTM

@flash1293 thank you for testing! I've added a retry logic to the k8s test, please take a look

@mykolaharmash mykolaharmash force-pushed the 199016-moving-tests-auto-detect-only branch 2 times, most recently from 341c705 to ea353e6 Compare December 11, 2024 15:09
@mykolaharmash mykolaharmash requested a review from a team as a code owner December 11, 2024 15:09
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, didn't test again but the change looks like what I had in mind

@mykolaharmash mykolaharmash force-pushed the 199016-moving-tests-auto-detect-only branch 2 times, most recently from b7f2c14 to dec550d Compare December 13, 2024 09:26
@mykolaharmash mykolaharmash requested a review from a team as a code owner December 13, 2024 09:26
@mykolaharmash mykolaharmash force-pushed the 199016-moving-tests-auto-detect-only branch from dec550d to de469e9 Compare December 13, 2024 09:37
@mykolaharmash mykolaharmash removed the request for review from a team December 13, 2024 13:42
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

packages/kbn-test/src/functional_test_runner/lib/config/run_check_ftr_configs_cli.ts changes LGTM

Copy link
Member

@wayneseymour wayneseymour left a comment

Choose a reason for hiding this comment

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

Left some questions.

testDir: './',
outputDir: './.playwright',
/* Run tests in files in parallel */
fullyParallel: true,
Copy link
Member

Choose a reason for hiding this comment

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

I see you'd like it to be fully parallel, which I believe is at the worker scope.
Is it ok that below workers is set to 1?

I guess I'm thinking how can it be fully parallel with one worker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Tbh, I did not write this config in the first place (migrated from the oblt-playwright repo), but I see this is the recommended basic config from the Playwright docs. I think it makes sense though in our use case, because tests are run both on the CI and locally, so there will be a single worker for the CI (AFAIK the instance running it does not have a lot of resources) and locally it will be the default value, which probably calculated depending on CPU cores count.

testIdAttribute: 'data-test-subj',
video: {
mode: 'off',
size: { width: 1920, height: 1200 },
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a size if this config is turned off?

Copy link
Contributor Author

@mykolaharmash mykolaharmash Dec 17, 2024

Choose a reason for hiding this comment

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

True, we don't need it, will remove the whole video block as it's off by default anyway

if (isAuthenticated) {
await page.context().storageState({ path: STORAGE_STATE });
} else if (spaceSelector) {
await page.locator('xpath=//a[contains(text(),"Default")]').click();
Copy link
Member

Choose a reason for hiding this comment

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

Why use an xpath locator if you've already added data-test-subj as the test id for the framework?

}

public readonly helpMenuButton = () =>
this.page.locator('xpath=//div[@data-test-subj="helpMenuButton"]');
Copy link
Member

Choose a reason for hiding this comment

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

Basically the same question as before, why use xpath? I could be wrong but I recall it being rather slow in the past and we've got a ton of issues with resources in testing.


private readonly useCaseKubernetes = () =>
this.page.locator(
'xpath=//div[@data-test-subj="observabilityOnboardingUseCaseCard-kubernetes"]//input[@type="radio"]'
Copy link
Member

Choose a reason for hiding this comment

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

You know, I just realized that our Team's POC is the only place I know of in the repo that has a playwright-data-test-subj integration.

Here it is.

In this case the kbn/scout package does not override playwrights test id attr, but this test subj functionality is exposed.

Maybe this is useful for you all?

\cc @dmlemeshko

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are good points about using xpath, I left them as they were originally written in the oblt-playwright repo, but it 100% makes sense to just use test id helper and narrow down using Locator's methods, I'll re-write those selector

…aywright

Update to the latest tests

Ignore playwright test results

Fix output directory

Temporarily move playwright config to the root of the repo

Enable screenshots

Add Kubernetes EA test

Reduce timeout for debugging
@mykolaharmash mykolaharmash force-pushed the 199016-moving-tests-auto-detect-only branch from 1ad6ae2 to 43fbac1 Compare December 17, 2024 15:44
@mykolaharmash
Copy link
Contributor Author

/oblt-deploy

@mykolaharmash mykolaharmash enabled auto-merge (squash) December 18, 2024 14:28
@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 18, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: ce7c493
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-203616-ce7c493e913b

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observabilityOnboarding 273.8KB 274.0KB +217.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/observability-onboarding-e2e 3 4 +1

Total ESLint disabled count

id before after diff
@kbn/observability-onboarding-e2e 3 4 +1

History

@mykolaharmash mykolaharmash merged commit 4bb6521 into elastic:main Dec 18, 2024
8 checks passed
JoseLuisGJ pushed a commit to JoseLuisGJ/kibana that referenced this pull request Dec 19, 2024
…ywright repo (elastic#203616)

Closes elastic#199016

This change migrates and and expands tests from
[oblt-playwright](https://github.com/elastic/oblt-playwright) repo.

These tests are part of the [Nightly
workflow](https://github.com/elastic/ensemble/actions/workflows/nightly.yml)
and being run by an Ensemble story on the CI.

The Nightly workflow itself is still in development and does not support
some of the use cases, that's why kubernetes tests are skipped for now
in this PR.

See the `./README.md` on how to run the tests locally.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
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 ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-logs Observability Logs User Experience Team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Observability Onboarding] Move Playwright tests to Kibana repo and setup local dev environment
6 participants