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

675 Add E2E Tests to CI without Mocking #676

Merged
merged 22 commits into from
Jun 26, 2024
Merged

Conversation

Joshua-Douglas
Copy link
Member

Closes #675.

What changes did you make?

  • Updated our CI pipeline to run our full e2e test suite against our real backend. This was most easily achieved using our docker container, since GitHub actions are able to manage container lifecycles when run using service containers.
  • Refactored the existing pipeline steps to use our docker images. This adds some automated build checks for our images, and simplifies the CI pipeline setup.
  • Add a health check endpoint to the API, and use it to verify that the API container startup was successful.
  • Fix minor bug in app/cypress/e2e/create-new-password.cy.ts. The test was short-circuiting the hostname config options.
  • bump cryptography to 42.0.4 since 41.0.7 has a known security vulnerability
    • cryptography is a transitive dependency of the moto library. Unfortunately the most recent versions of moto still use the outdated version so I had to elevate crypography to our pyproject.toml file.

It is worth noting that our API container is being used as an API and a database. In the future we might consider breaking out the database into a separate container, and populating it with some test data. I'm leaving as-is for now since our db is relatively simple at this point.

Rationale behind the changes?

  • Our real e2e tests have broken a few times because they are a pain to run locally. This CI pipeline will prevent PR merges that break our e2e tests.

Testing done for these changes

  • My biggest concern was verifying that the test-api-real-aws uses our real AWS service, and that test-app-real-backend uses the real backend. I verified both in d2e8836 by removing the AWS secret from the test config and remove the backend service from the CI pipeline. This change broke the "nomock" test suites, while the "mock" test suites continued to pass.
    • Also, the runtimes for the nomock test suites are significantly longer

What did you learn or can share that is new?(optional)

  • GitHub service containers make it very easy to orchestrate multiple services
  • The docker HEALTHCHECK command is very useful for running service containers with a lengthy startup process. GitHub action service containers will query the state of service containers using docker inspect. It does this query to determine when the container is ready for use. If there is no HEALTHCHECK then docker inspect returns a 0 exit code as soon as the container is initialized. A custom health check will ensure that docker inspect does not return 0 until the condition you define is met. In our case, the GH action should not progress until the API is actually available.

Screenshots of Proposed Change

Here is the new CI pipeline flow:

image

Copy link
Member

@paulespinosa paulespinosa left a comment

Choose a reason for hiding this comment

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

Lots of pros but has the downside that the runtime is longer. Find ways to utilized cache or stages.

raise AuthError({"message": msg}, 500)

def health():
Copy link
Member

Choose a reason for hiding this comment

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

Move to its own controller because it is not a responsibility of an admin.

@@ -1,7 +1,7 @@
# TODO: Run the openapi_server with nginx as reverse proxy and gunicorn as WSGI server
FROM python:3.10-alpine

RUN apk update && apk add build-base
RUN apk update && apk add build-base && apk add curl
Copy link
Member

@paulespinosa paulespinosa Apr 16, 2024

Choose a reason for hiding this comment

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

A shorter version: RUN apk --no-cache add build-base curl will get the latest package index without caching it in the image and install the two packages.

password: ${{ secrets.GITHUB_TOKEN }}
- name: Build and Push API Image
run: |
IMAGE_NAME=ghcr.io/hackforla/homeuniteus/api:latest
Copy link
Member

Choose a reason for hiding this comment

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

The tags are going to cause problems with parallel builds.

Copy link
Collaborator

@erikguntner erikguntner left a comment

Choose a reason for hiding this comment

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

@Joshua-Douglas This is really cool! It looks like the majority of the added time comes from building the images, so I wonder if there's a way to speed that up. Also, wondering if we need to run all of these checks on every PR or if some can be situational. Thoughts?

@johnwroge johnwroge requested a review from agosmou April 27, 2024 17:11
@johnwroge johnwroge removed the request for review from agosmou April 27, 2024 17:11
@Joshua-Douglas Joshua-Douglas marked this pull request as draft April 29, 2024 03:53
@Joshua-Douglas
Copy link
Member Author

I'm converting this to draft. I have some ideas on how to speed up the CI pipeline, but I'm going to implement #636 over these improvements.

@tylerthome tylerthome marked this pull request as ready for review June 26, 2024 01:04
@tylerthome tylerthome merged commit 04a7b48 into main Jun 26, 2024
1 of 2 checks passed
@tylerthome tylerthome deleted the 675-add-e2e-tests-to-ci branch June 26, 2024 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Real e2e Tests to the CI Pipeline
4 participants