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.

Sign up for GitHub

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

Jump to bottom

[APM] Stabilize cypress tests and move to datastreams #137977

Closed
wants to merge 5 commits into from
Closed

[APM] Stabilize cypress tests and move to datastreams #137977

wants to merge 5 commits into from

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Aug 3, 2022 •
edited

Closes #136627

Changes:

  • Enable datastreams for API and cypress tests
  • Stabilize cypress tests
    • Use cy.session for login
    • Rely on Cypress queue instead of manually wrapping in promises
    • Add retries to tests
    • Replace cy.visit with cy.visitKibana which waits for Kibana to have loaded before moving to the next step

Other improvements:

  • Ability to run Cypress from any location, eg node scripts/functional_tests.js --config=./x-pack/plugins/apm/ftr_e2e/ftr_config.ts

@sorenlouv sorenlouv force-pushed the use-datastreams-for-api-tests branch 2 times, most recently from c3ff9ef to a0b88ca Compare August 4, 2022 11:47
@sorenlouv
Copy link
Member Author

Failing test:



1) Service Overview
--
  | Calls APIs
  | when clicking the refresh button:
  | AssertionError: Timed out retrying after 15000ms: Expected to find content: 'Refresh' but never did.
  | at Context.eval (http://localhost:5620/__cypress/tests?p=cypress/integration/read_only_user/service_overview/service_overview.spec.ts:51272:16)
  |  
  |  
  |  
  |  
  | (Results)
  |  
  | ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  | │ Tests:        8                                                                                │
  | │ Passing:      4                                                                                │
  | │ Failing:      1                                                                                │
  | │ Pending:      3                                                                                │
  | │ Skipped:      0                                                                                │
  | │ Screenshots:  0                                                                                │
  | │ Video:        false                                                                            │
  | │ Duration:     40 seconds                                                                       │
  | │ Spec Ran:     read_only_user/service_overview/service_overview.spec.ts                         │
  | └────────────────────────────────────────────────────────────────────────────────────────────────┘

@sorenlouv sorenlouv force-pushed the use-datastreams-for-api-tests branch 4 times, most recently from 3dadb94 to 2f011f4 Compare August 8, 2022 14:09
@sorenlouv sorenlouv force-pushed the use-datastreams-for-api-tests branch from cc9014e to 6dfcf6b Compare August 12, 2022 12:14
sorenlouv
sorenlouv commented Aug 16, 2022
View reviewed changes
x-pack/plugins/apm/ftr_e2e/cypress/integration/power_user/feature_flag/comparison.spec.ts Outdated
Comment on lines 29 to 33
cy.loginAsEditorUser().then(() => {
// enables comparison feature on advanced settings
cy.updateAdvancedSettings({
'observability:enableComparisonByDefault': true,
});
});
Copy link
Member Author

Choose a reason for hiding this comment

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

This test was relying on the advanced setting observability:enableComparisonByDefault to be true. However, this is not always the case. We should write test so they can run independently of other tests.

cc @cauemarcondes

sorenlouv
sorenlouv commented Aug 16, 2022
View reviewed changes
x-pack/plugins/apm/ftr_e2e/cypress/integration/power_user/feature_flag/infrastructure.spec.ts Outdated
Comment on lines 17 to 18
before(async () => {
await synthtrace.index(
before(() => {
synthtrace.index(
Copy link
Member Author

@sorenlouv sorenlouv Aug 16, 2022 •
edited

Choose a reason for hiding this comment

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

We should no longer use await before synthtrace.clean or synthtrace.index since they are Cypress tasks handled by cypress own queueing mechanism

@sorenlouv sorenlouv force-pushed the use-datastreams-for-api-tests branch 4 times, most recently from e16fa9f to ea3389e Compare August 17, 2022 22:56
@sorenlouv sorenlouv marked this pull request as ready for review August 18, 2022 06:35
@sorenlouv sorenlouv force-pushed the use-datastreams-for-api-tests branch from 1912ad0 to 5fc0db9 Compare August 18, 2022 06:35
@sorenlouv sorenlouv requested a review from a team as a code owner August 18, 2022 06:35
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 18, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@sorenlouv sorenlouv added release_note:skip Skip the PR/issue when compiling release notes v8.5.0 labels Aug 18, 2022
@sorenlouv sorenlouv changed the title [APM] Enable datastreams for API and cypress tests [APM] Stabilize cypress tests and move to datastreams Aug 18, 2022
@sorenlouv sorenlouv enabled auto-merge (squash) August 18, 2022 08:47
@sorenlouv sorenlouv force-pushed the use-datastreams-for-api-tests branch 2 times, most recently from 9ed143d to 758c056 Compare August 18, 2022 10:11
gbamparop
gbamparop reviewed Aug 18, 2022
View reviewed changes
Copy link
Contributor

@gbamparop gbamparop left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, lots of good stuff here! I've added some comments on the PR.

x-pack/plugins/apm/ftr_e2e/cypress.json Show resolved Hide resolved
...gins/apm/ftr_e2e/cypress/integration/read_only_user/service_overview/instances_table.spec.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/ftr_e2e/cypress/support/index.ts Show resolved Hide resolved
x-pack/test/apm_api_integration/tests/infrastructure/infrastructure_attributes.spec.ts Show resolved Hide resolved
x-pack/plugins/apm/ftr_e2e/cypress/support/commands.ts Outdated Show resolved Hide resolved
x-pack/plugins/apm/ftr_e2e/cypress/plugins/index.ts Show resolved Hide resolved
x-pack/plugins/apm/scripts/test/e2e.js Show resolved Hide resolved
x-pack/plugins/apm/ftr_e2e/cypress/support/output_command_timings.ts Show resolved Hide resolved
@sorenlouv
Copy link
Member Author

@gbamparop All comments addressed

@gbamparop
Copy link
Contributor

@gbamparop All comments addressed

thanks for addressing those!

@sorenlouv sorenlouv force-pushed the use-datastreams-for-api-tests branch from 3a07c67 to a5d5e0d Compare August 18, 2022 23:48
@sorenlouv sorenlouv force-pushed the use-datastreams-for-api-tests branch from fe22788 to 428dd68 Compare August 22, 2022 08:58
@sorenlouv sorenlouv requested a review from a team as a code owner August 22, 2022 09:27
@sorenlouv sorenlouv force-pushed the use-datastreams-for-api-tests branch 2 times, most recently from d1ff437 to 10c897d Compare August 22, 2022 13:19
@sorenlouv sorenlouv force-pushed the use-datastreams-for-api-tests branch from 3d1c86e to bc704fb Compare August 23, 2022 07:34
@kibana-ci
Copy link
Collaborator

kibana-ci commented Aug 23, 2022 •
edited

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #36 / visualize app - new charts library visualize Timelion visualization should display correct data for specified index pattern and timefield

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/apm-synthtrace 74 75 +1

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/apm-synthtrace 11 12 +1
Unknown metric groups

API count

id before after diff
@kbn/apm-synthtrace 74 75 +1

History

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

@sorenlouv sorenlouv mentioned this pull request Aug 23, 2022
Merged
@sorenlouv
Copy link
Member Author

sorenlouv commented Aug 23, 2022 •
edited

Split into several smaller PRs:

@sorenlouv sorenlouv closed this Aug 23, 2022
auto-merge was automatically disabled August 23, 2022 10:09

Pull request was closed

This was referenced Aug 23, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbudz jbudz jbudz approved these changes

@gbamparop gbamparop gbamparop approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v8.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Move integration and e2e tests over to use datastreams
5 participants
@sorenlouv @elasticmachine @gbamparop @kibana-ci @jbudz