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

Sanlouise 11683 add connected apps cypress #13634

Merged
merged 5 commits into from Aug 11, 2020
Merged

Sanlouise 11683 add connected apps cypress #13634

merged 5 commits into from Aug 11, 2020

Conversation

sanlouise
Copy link
Contributor

Description

Add sufficient integration test coverage for connected apps.

Testing done

Works locally.

Screenshots

Acceptance criteria

  • Add cypress end to end tests for connected apps

Definition of done

  • Events are logged appropriately
  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

erikphansen
erikphansen reviewed Jul 30, 2020
View reviewed changes
Copy link
Contributor

@erikphansen erikphansen left a comment

Choose a reason for hiding this comment

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

Still poking around locally to see if I can do anything to help make the mobile tests less flaky but here are a couple little thoughts.

src/applications/personalization/profile-2/tests/e2e/connectedApps.cypress.spec.js Outdated
);

// Click on the disocnnect button of the first app
cy.contains('Disconnect').click();
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to use cy.findByRole('button', {name: /disconnect apple health from your account/i})

Copy link
Contributor

Choose a reason for hiding this comment

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

It's so weird that we don't need to force this click on mobile, too. I'm going to be happy when we figure out why the mobile menu trigger does not show up in time to "block" this click.

src/applications/personalization/profile-2/tests/e2e/connectedApps.cypress.spec.js Outdated

// Click in disconnect in the confirmation modal
cy.findByRole('button', { name: 'Disconnect' }).click();
cy.findByText(/processing update.../i).should('exist');
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally a matter of taste, but you could do a cy.findByRole('button', { name: 'Disconnect' }).should('not.exist') here to make sure the button you just clicked is gone.

src/applications/personalization/profile-2/tests/e2e/connectedApps.cypress.spec.js Outdated Show resolved Hide resolved
@sanlouise sanlouise force-pushed the sanlouise-11683-add-connected-apps-cypress branch from 33598df to 0eaf72a Compare August 3, 2020 21:33
erikphansen
erikphansen approved these changes Aug 3, 2020
View reviewed changes
src/applications/personalization/profile-2/tests/e2e/connectedApps.cypress.spec.js Outdated
);

// Click on the disocnnect button of the first app
cy.contains('Disconnect').click();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's so weird that we don't need to force this click on mobile, too. I'm going to be happy when we figure out why the mobile menu trigger does not show up in time to "block" this click.

@sanlouise sanlouise marked this pull request as ready for review August 4, 2020 01:53
@sanlouise sanlouise requested a review from a team August 4, 2020 01:53
@sanlouise sanlouise requested review from a team as code owners August 4, 2020 01:53
@sanlouise sanlouise force-pushed the sanlouise-11683-add-connected-apps-cypress branch 5 times, most recently from cd3ec80 to 5eb8f39 Compare August 4, 2020 19:03
@sanlouise sanlouise force-pushed the sanlouise-11683-add-connected-apps-cypress branch from dee3513 to 4b429ea Compare August 10, 2020 17:22
@va-vfs-bot va-vfs-bot temporarily deployed to master/sanlouise-11683-add-connected-apps-cypress August 10, 2020 18:59 Inactive
erikphansen
erikphansen reviewed Aug 11, 2020
View reviewed changes
src/applications/personalization/profile-2/tests/e2e/connectedApps.cypress.spec.js Outdated
@@ -0,0 +1,106 @@
import { PROFILE_PATHS } from '../../constants';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you rename this file to profile.connected-apps.cypress.spec.js to match the convention used by the other Cypress tests?

erikphansen
erikphansen reviewed Aug 11, 2020
View reviewed changes
src/applications/personalization/profile-2/tests/e2e/connectedApps.cypress.spec.js Outdated
cy.viewport('iphone-4');
}

cy.route({
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe move these route mocks up top with the mock GET?

erikphansen
erikphansen approved these changes Aug 11, 2020
View reviewed changes
Copy link
Contributor

@erikphansen erikphansen left a comment

Choose a reason for hiding this comment

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

Way to persevere :) My only real request is to change the name of the test file.

src/applications/personalization/profile-2/tests/e2e/connectedApps.cypress.spec.js Outdated
cy.findByRole('progressbar').should('exist');
cy.findByText(/loading your information/i).should('exist');
cy.findByText(/loading your information/i).should('not.exist');

Copy link
Contributor

Choose a reason for hiding this comment

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

could add a check that cy.findByRole('progressbar').should('not.exist') here

@va-vfs-bot va-vfs-bot temporarily deployed to master/sanlouise-11683-add-connected-apps-cypress August 11, 2020 15:16 Inactive
@sanlouise sanlouise merged commit d78a43e into master Aug 11, 2020
@sanlouise sanlouise deleted the sanlouise-11683-add-connected-apps-cypress branch August 11, 2020 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikphansen erikphansen erikphansen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sanlouise @erikphansen @va-vfs-bot