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

Attempt to speed up integration tests using pre-built images #2478

Open
wants to merge 58 commits into
base: master
from

Conversation

@chrisgilmerproj
Copy link
Contributor

commented Aug 1, 2019

Description

This changes the e2e tests in CircleCI to use docker-compose which will use pre-built images from AWS ECR to speed up testing. My tests show that this takes us from about 16 minutes for e2e tests to as low as 10 minutes (depending on how CircleCI is queuing containers).

Further speed ups could happen if we balance how many tests exist in each cypress test file. You can compare how big each test file is by using find cypress/integration/ -type f | xargs -I {} wc -l {} | sort. The mothballing of HHGs is going to remove all the TSP and some Office tests but we could still improve this a bit.

Also in this PR:

  • Removing docker-layer-caching in CircleCI to get a 30+ second speed up on steps that don't require it
  • The local Dockerfile.migrations_local more closely matches the Dockerfile.migrations and anything extra was moved to Dockerfile.tools (meaning we have higher fidelity in testing)
  • All the old Makefile targets for the e2e tests that touched the test database have been removed
  • An error was caught were setting MIGRATION_PATH with multiple ;; semicolons (ie empty path) would break the migrations.
  • docker-compose now uses port 4000 similar to cypress tests. That's because cypress doesn't make it easy to modify the expected port for these tests.

Setup

Running locally should be unaffected:

make e2e_test
make e2e_test_docker

Also, this change maintains the promise that what is run locally matches what will be run in CircleCI.

Code Review Verification Steps

  • Request review from a member of a different team.

@chrisgilmerproj chrisgilmerproj self-assigned this Aug 1, 2019

chrisgilmerproj added some commits Aug 1, 2019

@codecov

This comment has been minimized.

Copy link

commented Aug 1, 2019

Codecov Report

Merging #2478 into master will increase coverage by 0.1%.
The diff coverage is 33.4%.

@@           Coverage Diff            @@
##           master   #2478     +/-   ##
========================================
+ Coverage    60.6%   60.6%   +0.1%     
========================================
  Files         298     298             
  Lines       16240   16252     +12     
========================================
+ Hits         9833    9842      +9     
- Misses       5256    5258      +2     
- Partials     1151    1152      +1
Impacted Files Coverage Δ
pkg/cli/migration.go 40.8% <33.4%> (-0.1%) ⬇️
pkg/migrate/isAfterSpace.go 100% <0%> (ø) ⬆️
pkg/services/accesscode/fetch_access_code.go
pkg/services/accesscode/claim_access_code.go 100% <0%> (ø)

chrisgilmerproj added some commits Aug 1, 2019

Fix
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

I've done a little looking here and honestly this would probably work better if I used docker-compose.

chrisgilmerproj added some commits Aug 1, 2019

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

What this is missing is the ability to load the e2e test data.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Early testing shows that the CI stuff starts running in under 60 seconds (no data is generated for the DB yet!): https://circleci.com/gh/transcom/mymove/147727

I think that's promising for speeding up tests.

CONTAINER=mymove_milmove_1

# Need to wait not just for DB but also migrations to finish
DB_TIMEOUT=60 DB_PORT=6432 DB_NAME=dev_db scripts/wait-for-db

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Aug 1, 2019

Author Contributor

I constantly forget that CircleCI doesn't let you talk to the running instances via localhost. So the only way to talk is via another container you bring up.

# Fill with test data
psql postgres://postgres:mysecretpassword@localhost:6432/dev_db?sslmode=disable -c 'TRUNCATE users CASCADE;'
make bin/generate-test-data
bin/generate-test-data --named-scenario="e2e_basic" --db-env="development" --db-port 6432

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Aug 1, 2019

Author Contributor

So while this works locally we'd actually have to do this inside docker to make it work.

chrisgilmerproj added some commits Aug 1, 2019

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

I'm beginning to think that pushing a new tools docker container to AWS makes sense. It would be pretty trivial to do and it would speed things up a lot. We're already building all those tools so why not put them in a useful place?

@chrisgilmerproj chrisgilmerproj marked this pull request as ready for review Aug 21, 2019

@chrisgilmerproj chrisgilmerproj requested review from mr337, jim, lynzt and rdhariwal Aug 21, 2019

@mr337

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Kicking off another CI build since it doesn't seem to be related to this PR.

@chrisgilmerproj chrisgilmerproj requested a review from garrettqmartin8 Aug 22, 2019

@garrettqmartin8

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

@chrisgilmerproj I get this error when running make e2e_test_docker:

Successfully built 2e59f13b51f9
Successfully tagged mymove_milmove_migrate:latest
Building milmove
Step 1/10 : FROM gcr.io/distroless/base:latest
 ---> 7f04a8d24717
Step 2/10 : COPY bin/rds-combined-ca-bundle.pem /bin/rds-combined-ca-bundle.pem
ERROR: Service 'milmove' failed to build: COPY failed: stat /var/lib/docker/tmp/docker-builder362986316/bin/rds-combined-ca-bundle.pem: no such file or directory
make: *** [e2e_test_docker] Error 1

Am I missing a setup step?

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

Step 2/10 : COPY bin/rds-combined-ca-bundle.pem /bin/rds-combined-ca-bundle.pem

@garrettqmartin8 Discovered I used Dockerfile instead of Dockerfile.local for this. You don't need the RDS certs for local development and testing so I'll push out a fix shortly. If only I'd hit make clean e2e_test_docker I would have caught this!

@garrettqmartin8
Copy link
Contributor

left a comment

Able to run make e2e_test_docker now. Good stuff @chrisgilmerproj!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.