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] Fix broken e2e tests #129475

Merged
merged 29 commits into from
Apr 14, 2022
Merged

[APM] Fix broken e2e tests #129475

merged 29 commits into from
Apr 14, 2022

Conversation

gbamparop
Copy link
Contributor

@gbamparop gbamparop commented Apr 5, 2022 •
edited
Loading

Summary

  • Add option to run APM cypress tests in the flaky test runner
  • Add change to swallow all uncaught exceptions
  • Fix the broken e2e tests that are skipped
    • x-pack/plugins/apm/ftr_e2e/cypress/integration/read_only_user/home.spec.ts
    • x-pack/plugins/apm/ftr_e2e/cypress/integration/read_only_user/service_overview/service_overview.spec.ts
    • x-pack/plugins/apm/ftr_e2e/cypress/integration/read_only_user/service_overview/time_comparison.spec.ts

Addresses part of #128752

@gbamparop gbamparop added Team:APM All issues that need APM UI Team support release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v8.2.0 labels Apr 5, 2022
@gbamparop gbamparop changed the title [APM] Fix skipped e2e test [APM] Fix skipped e2e tests Apr 5, 2022
gbamparop
gbamparop commented Apr 6, 2022
View reviewed changes
x-pack/plugins/apm/ftr_e2e/cypress/support/commands.ts
.type(moment(start).format(format), { force: true });
cy.get('[data-test-subj="superDatePickerendDatePopoverButton"]').click();
cy.get('[data-test-subj="superDatePickerAbsoluteDateInput"]')
.eq(1)
.clear()
.clear({ force: true })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This element is not visible and the type below results in invalid date

gbamparop
gbamparop commented Apr 6, 2022
View reviewed changes
...gins/apm/ftr_e2e/cypress/integration/read_only_user/service_overview/time_comparison.spec.ts
cy.contains('Day before');
cy.contains('Week before');

cy.selectAbsoluteTimeRange(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to use absolute dates to calculate the period consistently.

gbamparop
gbamparop commented Apr 6, 2022
View reviewed changes
...s/apm/ftr_e2e/cypress/integration/read_only_user/service_inventory/service_inventory.spec.ts Outdated
});

it('with the correct environment when changing the environment', () => {
cy.wait(aliasNames);
cy.wait(aliasNames, { requestTimeout: 10000 });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even with 10000 they fail sometimes

Copy link
Member

Choose a reason for hiding this comment

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

any idea why? is it because they finish slightly later? or they're not captured at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't managed to reproduce it locally and can't tell from the CI failure

Copy link
Member

Choose a reason for hiding this comment

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

hmm, is there any way we can figure it out? maybe add & log some request timings for the time being?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've increased the requestTimeout to 30s and temporarily removed cypress retries in 761dc8e and in 50 test runs it doesn't seem to have failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually one is still failing :|

gbamparop
gbamparop commented Apr 6, 2022
View reviewed changes
x-pack/plugins/apm/ftr_e2e/cypress/integration/read_only_user/home.spec.ts Outdated
cy.visit(serviceInventoryHref);

cy.contains('Services');
cy.contains('opbeans-rum').wait(3000).click({ force: true });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Waiting on DOM elements is discouraged but click causes the test to be flaky. As we've seen this quite a few times so far, we might benefit by using cypress-pipe (also used by security)

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we wait on API calls to complete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could but not sure if this will change it. opbeans-rum is there, it's just the click that is failing

Copy link
Member

Choose a reason for hiding this comment

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

is it because the table is still in a loading state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be, also I've read that some DOM-related flaky tests might be due to components re-rendering after cy.contains and before click causing such issues.

gbamparop
gbamparop commented Apr 6, 2022
View reviewed changes
x-pack/plugins/apm/ftr_e2e/cypress/integration/read_only_user/home.spec.ts
@@ -47,12 +41,12 @@ describe.skip('Home page', () => {
cy.loginAsReadOnlyUser();
});

it('Redirects to service page with environment, rangeFrom and rangeTo added to the URL', () => {
it('Redirects to service page with comparisonEnabled, environment, rangeFrom, rangeTo and offset added to the URL', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More redirects have been added by #127812

gbamparop
gbamparop commented Apr 6, 2022
View reviewed changes
x-pack/plugins/apm/ftr_e2e/cypress/integration/power_user/feature_flag/infrastructure.spec.ts
@@ -65,6 +65,7 @@ describe('Infrastracture feature flag', () => {
it('shows infrastructure tab in service overview page', () => {
cy.visit(serviceOverviewPath);
cy.contains('a[role="tab"]', 'Infrastructure').click();
cy.contains('Infrastructure data coming soon');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An expectation was added to be more explicit

@gbamparop gbamparop marked this pull request as ready for review April 11, 2022 14:14
@gbamparop gbamparop requested review from a team as code owners April 11, 2022 14:14
@elasticmachine
Copy link
Contributor

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

@gbamparop gbamparop changed the title [APM] Fix skipped e2e tests [APM] Reduce the number of flaky e2e tests Apr 11, 2022
@gbamparop
Copy link
Contributor Author

@elasticmachine merge upstream

@gbamparop gbamparop changed the title [APM] Reduce the number of flaky e2e tests [APM] Fix broken e2e tests Apr 11, 2022
@cauemarcondes
Copy link
Contributor

I think I fixed the ftr_e2e/cypress/integration/read_only_user/service_overview/header_filters.spec.ts.

Screen Shot 2022-04-11 at 4 21 09 PM

@gbamparop
Copy link
Contributor Author

@elasticmachine merge upstream

dgieselaar
dgieselaar reviewed Apr 12, 2022
View reviewed changes
x-pack/plugins/apm/ftr_e2e/cypress/integration/read_only_user/home.spec.ts

// flaky test
describe.skip('Home page', () => {
Copy link
Member

Choose a reason for hiding this comment

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

should we unskip this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still flaky

dgieselaar
dgieselaar reviewed Apr 12, 2022
View reviewed changes
...s/apm/ftr_e2e/cypress/integration/read_only_user/service_inventory/service_inventory.spec.ts
@@ -77,11 +75,14 @@ describe('When navigating to the service inventory', () => {
cy.contains('h1', 'opbeans-node');
});

describe('Calls APIs', () => {
describe.skip('Calls APIs', () => {
Copy link
Member

Choose a reason for hiding this comment

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

can you leave a comment here to explain why it's skipped (and maybe reference an issue)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it was failing while waiting for the API calls to finish

dgieselaar
dgieselaar reviewed Apr 12, 2022
View reviewed changes
...ugins/apm/ftr_e2e/cypress/integration/read_only_user/service_overview/header_filters.spec.ts
describe('Filtering by transaction type', () => {
beforeEach(() => {
Copy link
Member

Choose a reason for hiding this comment

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

what's the purpose of moving this into both child test suites? was this supposed to be a before()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cauemarcondes

Copy link
Contributor

Choose a reason for hiding this comment

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

@dgieselaar I noticed that once in a while a test fails because cypress logs kibana out, so to avoid such a thing I decided to move the login function to inside each describe.

dgieselaar
dgieselaar approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

some comments about some remaining skipped tests, but LGTM other than that

@gbamparop
Copy link
Contributor Author

I think I fixed the ftr_e2e/cypress/integration/read_only_user/service_overview/header_filters.spec.ts.

Thanks for having a look, it seems that it can still be flaky

@cauemarcondes
Copy link
Contributor

I think I fixed the ftr_e2e/cypress/integration/read_only_user/service_overview/header_filters.spec.ts.

Thanks for having a look, it seems that it can still be flaky

wow, that's so weird, it passed 50 times on my env.

@gbamparop
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] OSS Firefox Tests / discover app discover tab field data shows top-level object keys

Metrics [docs]

✅ unchanged

History

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

@gbamparop gbamparop merged commit f0553d3 into elastic:main Apr 14, 2022
kibanamachine pushed a commit that referenced this pull request Apr 14, 2022
@gbamparop
[APM] Fix broken e2e tests (#129475)
032327f 
* Fix skipped e2e tests

* Add expectation for the infrastructure tab

* Add option to run APM cypress tests in the flaky test runner

Co-authored-by: cauemarcondes <caue.marcondes@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit f0553d3)
@kibanamachine kibanamachine mentioned this pull request Apr 14, 2022
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.2

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Apr 14, 2022
@kibanamachine @gbamparop
[APM] Fix broken e2e tests (#129475) (#130329)
3ba2c5c 
* Fix skipped e2e tests

* Add expectation for the infrastructure tab

* Add option to run APM cypress tests in the flaky test runner

Co-authored-by: cauemarcondes <caue.marcondes@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit f0553d3)

Co-authored-by: Giorgos Bamparopoulos <georgios.bamparopoulos@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cauemarcondes cauemarcondes cauemarcondes left review comments

@dgieselaar dgieselaar dgieselaar approved these changes

@jbudz jbudz jbudz approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gbamparop @cauemarcondes @elasticmachine @kibana-ci @kibanamachine @dgieselaar @jbudz