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

Enhancement/update critical flows with e2e test coverage #87

Open
wants to merge 96 commits into
base: develop
Choose a base branch
from

Conversation

MaxwellGarceau
Copy link
Collaborator

@MaxwellGarceau MaxwellGarceau commented Dec 23, 2024

Description of the Change

Update E2E Cypress tests to cover all critical testing flows.

Enhancement/add mailchimp api client - #99

  • Add a FE Mailchimp client to Cypress tests to verify Mailchimp account interactions. Ensures successful form submission and full integration testing.
  • Use .env.test for env variables
  • Add health check for tests if Mailchimp client is unable to make requests
  • Modularize Cypress commands

‼️ Environment changes needed ‼️

  • Need to generate an API key for the test Mailchimp user in the CI/CD pipeline.
  • Need to attach that API key to CYPRESS_MAILCHIMP_API_KEY as a Github secret
  • Need to attach the server prefix to CYPRESS_MAILCHIMP_API_SERVER_PREFIX as a Github secret

Closes #61
Closes #86

How to test the Change

Follow the E2E testing instructions in the readme to run the Cypress tests. In addition, these tests should run automatically as part of the projects CI/CD flow.

Changelog Entry

Added - New tests to cover E2E critical flows
Changed - Existing tests to more robustly cover E2E critical flows. Modularized reusable testing code.

Credits

Props @MaxwellGarceau

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@github-actions github-actions bot added this to the 1.7.0 milestone Dec 23, 2024
Highlights
- Add a FE Mailchimp client to Cypress tests to verify Mailchimp account interactions. Ensures successful form submission and full integration testing.
- Use .env.test for env variables
- Add health check for tests if Mailchimp client is unable to make requests
- Modularize Cypress commands

‼️ Environment changes needed ‼️
- Need to generate an API key for the test Mailchimp user in the CI/CD pipeline.
- Need to attach that API key to CYPRESS_MAILCHIMP_API_KEY as a Github secret
- Need to attach the server prefix to CYPRESS_MAILCHIMP_API_SERVER_PREFIX as a Github secret

Commits

* Add Mailchimp API package

* Add cypress-dotenv

* Add Mailchimp API client to Cypress

Necessary to interact with the test user account to verify contacts, make deletions, etc.

* Update documentation for env setup

* Remove outdated E2E notes

* Remove log to fix bug. Skip unfinished tests.

* Remove outdated readme note

* Add test to verify lists in WP admin match lists in Mailchimp account

* Add assertion that contacts in Mailchimp account match email submitted

* Simplify mailchimp commands

* Add mailchimp commands to command folder

* Organize commands into modular files

* Rename mailchimpApiConfig to more clearly state purpose

* Fix grammar mistake

* Add scaffolding for gmail api

* Switch to gabi-script for FE requests

* Update .env.test.sample with example of google creds

* Remove gmail api client

Too complicated to set up for our purposes. We're better off with Mailhog

* Remove gapi-script
Add support for fetching more contacts

Add support for fetching contacts by status

Add utility function to set boolean settings
Skip tests that currently test bugged functionality
This test was decided to be a 'doing it wrong' site owner scenario. Any use of the plugin would require saving a list and therfore this is not a test we will be including
@MaxwellGarceau
Copy link
Collaborator Author

Status

@jeffpaul and @dkotter E2E tests passing with headless Cypress 🙂 - npm run cypress:run

e2e-tests-passing-headless

Critical flows checklist

I duplicated the critical flows checklist and added ✅, ⚠️, ‼️ as well as notes to designate ✅ completed tests, ⚠️ tests that we won't write E2E tests for or can't due to some limitation, ‼️ and tests that are disabled or can't be written due to a known bug.

Known problems

  • We can't write tests that require verifying confirmation emails because we need an email server that allows querying the emails via API
  • Sometimes when running Cypress in GUI mode the web browser will crash
  • In Cypress GUI the test spec file will sometimes be 302 redirected back to the WP login. Clearing the session data solves the problem, but it's very frustrating
  • Everything has been going well with the Mailchimp API, but as Darin mentioned in Update critical flows with e2e test coverage #61 (comment) we should keep an eye on possible failures due to throttling or rate limits from Mailchimp's API. The only restriction I could find was that we can't have more than 10 simultaneous requests.

Next Steps

  • I'd like to make a revision to the original testing instructions for critical flows to add my notes regarding E2E tests in order to highlight areas that will need to be QA'd and UAT'd with more scrutiny.
  • Environment variables we need available in Github
    • CYPRESS_MAILCHIMP_API_KEY - API generated key from the Mailchimp account tied to the user
    • CYPRESS_MAILCHIMP_API_SERVER_PREFIX - this value will be appended to the API key - e.g. "us19" - docs
  • cypress-dotenv and .env.test - I added cypress-dotenv for easier local testing. I wanted to call this out in case anything extra is needed in the CI/CD pipeline.

After we add the environment variables for the API key I can fix any errors that come up in the CI/CD pipeline.

@MaxwellGarceau MaxwellGarceau marked this pull request as ready for review January 11, 2025 01:43
@github-actions github-actions bot added the needs:code-review This requires code review. label Jan 11, 2025
Caused an error that caused session data to malfunction and fail every other test next in the cycle
.scrollIntoView({ offset: { top: -100, left: 0 } })
.should('be.visible') // Check if the button is visible
.and('not.be.disabled'); // Ensure the button is not disabled
// .click(); // Perform the click action
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be removed if we're not using it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Originally, there was a blocker where we couldn't submit the same email repeatedly because we had no way of deleting the contact in Mailchimp after submission. However, now that we have the command cy.deleteContactFromList we could easily clean up after running .click().

So we could either:

  1. Remove the submission step
  2. Enable .click(), add a successful form submission assertion, and then delete the contact in the Mailchimp account

What do you think? I could see the argument for both approaches being valid.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's probably fine to just remove that submission step for now

@github-actions github-actions bot added the needs:refresh This requires a refreshed PR to resolve. label Jan 28, 2025
Copy link

@MaxwellGarceau thanks for the PR! Could you please rebase your PR on top of the latest changes in the base branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review. needs:refresh This requires a refreshed PR to resolve.
Projects
None yet
2 participants