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

Add e2e for the apm integration policy form #129860

Merged
merged 8 commits into from
Apr 12, 2022
Merged

Add e2e for the apm integration policy form #129860

merged 8 commits into from
Apr 12, 2022

Conversation

kpatticha
Copy link
Contributor

@kpatticha kpatticha commented Apr 10, 2022 •
edited
Loading

Summary

The e2e tests are focused on the apm integration.
closes #123399

@kpatticha kpatticha requested a review from a team as a code owner April 10, 2022 21:36
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Apr 10, 2022
@elasticmachine
Copy link
Contributor

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

@kpatticha
Copy link
Contributor Author

@elasticmachine merge upstream

@kpatticha kpatticha added backport:skip This commit does not require backporting v8.2.0 release_note:skip Skip the PR/issue when compiling release notes labels Apr 10, 2022
kpatticha
kpatticha commented Apr 11, 2022
View reviewed changes
...lugins/apm/ftr_e2e/cypress/integration/power_user/integration_settings/integration_policy.ts Outdated
cy.get('[data-test-subj="packagePolicyUrlInput').clear().type(url);
});

it('checks validators for required fields', () => {
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 checked only the required fields here. I think I should write different tests for the form itself.
Let me know your thoughts

Copy link
Member

Choose a reason for hiding this comment

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

I think having an E2E test that asserts that validation is working in general, and then covering the remaining areas with unit tests, is a good balance.

@kpatticha kpatticha changed the title Add apm integration test Add e2e for the apm integration policy form Apr 12, 2022
dgieselaar
dgieselaar reviewed 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.

forgot to press submit on a pending review :)

...lugins/apm/ftr_e2e/cypress/integration/power_user/integration_settings/integration_policy.ts Outdated
});

describe('creating a new integration policy', () => {
beforeEach(() => {
Copy link
Member

Choose a reason for hiding this comment

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

This feels llike it's mixing assertions with input. IMHO the assertions belong in a test, the input in the body of a beforeEach

Copy link
Contributor Author

@kpatticha kpatticha Apr 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, it seems cleaner the assertions to be in the test
1394538

...lugins/apm/ftr_e2e/cypress/integration/power_user/integration_settings/integration_policy.ts Outdated
Comment on lines 24 to 29
cy.get('[data-test-subj="integration-card:epr:apm:featured').click();
cy.get('[aria-selected="true"]').contains('Elastic APM in Fleet');
cy.contains('Elastic APM now available in Fleet!');
cy.contains('a', 'APM integration').click();
cy.url().should('include', 'app/integrations/detail/apm/overview');
cy.get('[data-test-subj="addIntegrationPolicyButton"]')
Copy link
Member

Choose a reason for hiding this comment

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

it's a little hard for me to understand what these statements are actually doing. perhaps it would be benefit by abstracting them into functions or leaving comments?

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 updated it should be more obvious now
#1394538

...lugins/apm/ftr_e2e/cypress/integration/power_user/integration_settings/integration_policy.ts Outdated
cy.get('[data-test-subj="packagePolicyUrlInput').clear().type(url);
});

it('checks validators for required fields', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think having an E2E test that asserts that validation is working in general, and then covering the remaining areas with unit tests, is a good balance.

@kpatticha kpatticha requested a review from a team as a code owner April 12, 2022 12:17
cauemarcondes
cauemarcondes approved these changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Test Failures

  • [job] [logs] OSS CI Group #11 / visualize app visualize ciGroup11 visual builder Time Series basics Clicking on the chart should create a filter for series with multiple split by terms fields one of which has formatting

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
apm 2.8MB 2.8MB +235.0B

History

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

@kpatticha kpatticha merged commit 8022b0a into elastic:main Apr 12, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 13, 2022
@jloleysens
Merge branch 'main' of github.com:elastic/kibana into reporting/soft-…
26b64f9 
…disable-server-side

* 'main' of github.com:elastic/kibana: (35 commits)
  [Uptime] remove latency limit warnings when using monitor management (elastic#129597)
  [Security Solution] [ReponseOps] Executes Cases Cypress test when there is a change on cases plugin (elastic#129992)
  Paramaterized Discover tests (elastic#129684)
  [Security Solution][Investigations] - Minor bug fixes (elastic#130054)
  [DOCS} Adds technical preview to Lens annotations (elastic#130058)
  [Security solution] [Endpoint] Revisit blocklist wrong labels (elastic#128773)
  [Security Solutions] Adds API docs for value lists (elastic#129962)
  [CI] Move jest tests to spot instances, and fix spot retries in PRs (elastic#130045)
  chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130051)
  [SecuritySolution] Remove the cell hovers actions for agent status (elastic#130042)
  Upgrade RxJS to 7 (elastic#129087)
  [SecuritySolution] Clean up CaseContext (elastic#130036)
  Revert "chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)"
  Use RuleDataReader to query for threshold signal history (elastic#129763)
  Remove securityRulesCancelEnabled setting and set shorter default timeouts (elastic#129769)
  Upgrade EUI to v54.0.0 (elastic#129653)
  [Security Solution] More Ransomware exceptionable fields (elastic#130039)
  Add e2e for the apm integration policy form (elastic#129860)
  chore(NA): upgrades rules_node_js to v5.4.0 (elastic#130021)
  [ML] Fix Single Metric Viewer chart failing to load if no points during calendar event (elastic#130000)
  ...

# Conflicts:
#	x-pack/plugins/screenshotting/server/screenshots/index.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgieselaar dgieselaar dgieselaar left review comments

@jbudz jbudz jbudz approved these changes

@cauemarcondes cauemarcondes cauemarcondes approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting 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.

[APM] Write browser tests for the APM Create/Edit Integration Settings
6 participants
@kpatticha @elasticmachine @kibana-ci @dgieselaar @jbudz @cauemarcondes