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

Running Cypress in Docker #3688

Merged
merged 18 commits into from
Aug 17, 2022
Merged

Running Cypress in Docker #3688

merged 18 commits into from
Aug 17, 2022

Conversation

alismx
Copy link
Collaborator

@alismx alismx commented Apr 21, 2022

Cypress PULL REQUEST

resolves #1770
resolves #3280
resolves #3637

Related Issue

  • Clean up cypress implementation
  • Make use of standard docker setup
  • Increase integration testing coverage of a live environment

Changes Proposed

  • Run Cypress in docker
  • Add Test to our ci pipeline
  • Run Cypress against a remote environment
  • Move Cypress directory out of the frontend directory
  • Update Cypress tests to handle more variable data
  • Handle cypress user having access to multiple facilities
  • Update Okta mocks for local runs
  • Update .env files for use in docker
  • Parameterize okta request for authenication
  • Block staging/prod deploys if cypress fails against test

Additional Information

  • Add swap space to the local run is pretty straightforward and seems to have had a positive impact on preventing Cypress from hanging and other failures, I used an open source repo to do this but could easily add a new action. Open to preferences and thoughts.
  • It would be nice to decouple wiremock from running in the cypress container

Testing

Checklist for Primary Reviewer

Infrastructure

  • Consult the results of the terraform-plan job inside the "Terraform Checks" workflow run for this PR. Confirm that there are no unexpected changes!

Security

  • Changes with security implications have been approved by a security engineer (changes to authentication, encryption, handling of PII, etc.)
  • Any dependencies introduced have been vetted and discussed

Cloud

  • If there are changes that cannot be tested locally, this has been deployed to our Azure test, dev, or pentest environment for verification

@alismx alismx force-pushed the alis/1770-3280 branch 3 times, most recently from 93bd050 to 91f6ca6 Compare April 22, 2022 23:19
@alismx alismx temporarily deployed to Test April 22, 2022 23:27 Inactive
@alismx alismx temporarily deployed to Test April 23, 2022 00:01 Inactive
@alismx alismx temporarily deployed to Test April 23, 2022 00:27 Inactive
@alismx alismx temporarily deployed to Test April 23, 2022 01:13 Inactive
@alismx alismx temporarily deployed to Test April 23, 2022 04:19 Inactive
@alismx alismx force-pushed the alis/1770-3280 branch 4 times, most recently from 78b2a6d to a92d049 Compare April 23, 2022 05:26
@alismx alismx temporarily deployed to Test April 23, 2022 05:32 Inactive
@alismx alismx force-pushed the alis/1770-3280 branch 2 times, most recently from 2cb95c2 to 554c13c Compare April 23, 2022 05:51
@alismx alismx temporarily deployed to Test April 23, 2022 05:57 Inactive
@alismx alismx temporarily deployed to Test April 25, 2022 17:03 Inactive
@alismx alismx temporarily deployed to Test April 25, 2022 17:37 Inactive
@alismx alismx temporarily deployed to Test April 25, 2022 18:50 Inactive
@alismx alismx temporarily deployed to Test April 25, 2022 19:53 Inactive
@alismx alismx dismissed rin-skylight’s stale review August 11, 2022 18:39

This has been tested by 3 devs locally.

#!/bin/bash

echo "git config --global --add safe.directory /app"
git config --global --add safe.directory /app
Copy link
Contributor

@georgehager georgehager Aug 11, 2022

Choose a reason for hiding this comment

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

What does this do? Curious what problem it resolves

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a workaround to a reported bug, it resolves a "dubious" permissions error when running the frontend with docker. We run a git command on frontend startup and this is required to do that.

@@ -5,14 +5,14 @@
"url" : "/api/v1/users?activate=false",
"method" : "POST",
"bodyPatterns" : [ {
"equalToJson" : "{\"profile\":{\"firstName\":\"Greg\",\"lastName\":\"McTester\",\"login\":\"[email protected]\",\"email\":\"[email protected]\"},\"groupIds\":[\"00g228nyd7uuGaG1L1d7\",\"00g228nrj9QDJ10y61d7\"]}",
"equalToJson" : "{\"profile\":{\"firstName\":\"Greg\",\"lastName\":\"McTester\",\"login\":\"${json-unit.any-string}\",\"email\":\"${json-unit.any-string}\"},\"groupIds\":[\"00g228nyd7uuGaG1L1d7\",\"00g228nrj9QDJ10y61d7\"]}",
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a neat trick!

Copy link
Contributor

Choose a reason for hiding this comment

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

thought I'm realizing this is in an auto-generated file 🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

@georgehager mostly auto-generated! It's created by WireMock but this any-string piece is hand-touched :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was a manual update to allow for any email. Doing so lets us add more people and not need to clean the db every time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Neat! I saw in the test we type in user.email so that makes sense!

- Run Cypress locally and open interactive mode. Do this if you're running the apps locally with docker-compose with Okta enabled.
- `yarn e2e:nginx:okta`

### Potential issues:
Copy link
Contributor

Choose a reason for hiding this comment

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

Super helpful!

.gitignore Outdated Show resolved Hide resolved
Copy link
Contributor

@georgehager georgehager left a comment

Choose a reason for hiding this comment

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

Looks great! this will make running e2es locally so much easier 🙌

@georgehager
Copy link
Contributor

@alismx did you decide not to add a command for running all e2e tests headless locally outside of docker? It's something I imagine wanting to do occasionally, so I don't think it's super critical and might not be worth the effort of adding another separate command since they can all be run in open mode.

@alismx
Copy link
Collaborator Author

alismx commented Aug 11, 2022

@alismx did you decide not to add a command for running all e2e tests headless locally outside of docker? It's something I imagine wanting to do occasionally, so I don't think it's super critical and might not be worth the effort of adding another separate command since they can all be run in open mode.

What do you say about making a follow-up ticket? I can write that up and link it here.

@georgehager
Copy link
Contributor

georgehager commented Aug 11, 2022

What do you say about making a follow-up ticket? I can write that up and link it here.

Yep I think that's great! Like I said, not super critical 😁

Copy link
Collaborator

@emyl3 emyl3 left a comment

Choose a reason for hiding this comment

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

Couple questions for you alis! Thank you for your all your work on this! 😭 🙌

CYPRESS_OKTA_SECRET=
CYPRESS_OKTA_REDIRECT_URI="https%3A%2F%2Ftest.simplereport.gov%2Fapp%2F"
CYPRESS_OKTA_SCOPE="simple_report_test"
CYPRESS_OKTA_CLIENT_ID="0oa1khettjHnj3EPT1d7"
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this ok to have checked in? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Committing this value to the repo is fine. The client id is very easy to find in our auth flow as it gets passed around in the URL of the auth requests. We don't want to expose API keys or secrets.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it! Thank you @alismx :D

.github/workflows/deployTest.yml Show resolved Hide resolved
cypress/support/commands.js Show resolved Hide resolved
@@ -0,0 +1,123 @@
## E2E tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

thank you for so thoroughly updating this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😅

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, this is awesome! Once this is merged, would you mind also adding it to the wiki? (or just linking to this README somewhere on our getting started wiki page)

Copy link
Collaborator

@emyl3 emyl3 left a comment

Choose a reason for hiding this comment

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

LGTM!! Thank you!! 😹

Copy link
Contributor

@emmastephenson emmastephenson left a comment

Choose a reason for hiding this comment

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

Looks awesome Alis, thanks for all your work on this! 🙌

runs-on: ubuntu-latest
needs: [deploy]
defaults:
run:
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why we're no longer verifying the release for dev?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This check is the first test of the cypress suite, which happens locally during the PR process and against the test environment once a merge goes through. This was only in place until we could run the whole suite against a remote environment in its entirety.

I wouldn't mind this type of check for all of our remote environments, but it does take some time because of its dependence on spinning up cypress.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for the explanation!

@@ -0,0 +1,123 @@
## E2E tests
Copy link
Contributor

Choose a reason for hiding this comment

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

+1, this is awesome! Once this is merged, would you mind also adding it to the wiki? (or just linking to this README somewhere on our getting started wiki page)

@alismx
Copy link
Collaborator Author

alismx commented Aug 15, 2022

I can't seem to merge this until the sonar job is fixed.

@snesm
Copy link
Contributor

snesm commented Aug 17, 2022

@alismx, it looks like "test Expected — Waiting for status to be reported" is the hold up. The sonar check is not "required"

@rin-skylight
Copy link
Collaborator

@snesm Sonar is the issue. There is a dependent check in the "test" workflow. PR is already in with the bypass.

@alismx
Copy link
Collaborator Author

alismx commented Aug 17, 2022

@snesm @rin-skylight @emmastephenson Not sure what I'm missing, but it seems like this "test" check is only blocking this PR. I thought it was because of the Sonar issue failing, but now that we've merged in that sonar bypass, I believe that's been ruled out. Has anybody seen this before? Does anybody see any issues with the github workflows in this PR?

@rin-skylight
Copy link
Collaborator

@alismx This can happen on occasion. Creating a PR with a new branch, that has all of your existing changes, will likely be the easiest thing to kick the checks into gear. I've forced a re-run of the test workflow, but that reporting hook still might not function correctly.

@alismx alismx closed this Aug 17, 2022
@alismx alismx reopened this Aug 17, 2022
@alismx alismx merged commit 80a6dae into main Aug 17, 2022
@alismx alismx deleted the alis/1770-3280 branch August 17, 2022 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants