\#1500 - prototype for using testcontainers-go for dependencies #1544

ClaytonNorthey92 · 2020-02-16T19:40:49Z

What is the problem I am trying to address?

This is meant to be a prototype for using testcontainers for dependencies such as redis, minio, and mongo.

How is the fix applied?
In the tests, rather than checking if an env variable is set to run tests against these dependencies, I start docker containers with testcontainers in the tests themselves

Mention the issue number it fixes or add the details of the changes if it doesn't have a specific issue.
This prototype was mentioned in #1500 , this should not be merged (yet), it is just a proof of concept.

Fixes #1500

Edit by @arschles - added Fixes to this PR description so that it closes the issue when it's merged

e2etests/all_test.go

.drone.yml

appveyor.yml

bsideup · 2020-02-17T07:43:01Z

cc @gianarb

arschles

@ClaytonNorthey92 thanks for this! it's looking very promising so far 🎉. One note - we will need to change some of the local development documentation, the Makefile and the docker compose file - which is also used primarily for local development. Since the tests themselves are starting up all of the service dependencies, we won't need the Makefile/docker compose to do it before running tests.

cc/ @fedepaol because you were in the original discussion as well

.drone.yml

appveyor.yml

ClaytonNorthey92 · 2020-02-20T05:59:45Z

@arschles I have added the workflows directory, based off of your gh-actions branch, here is the latest action running the tests (with testcontainers)

ClaytonNorthey92 · 2020-02-20T06:01:37Z

@ClaytonNorthey92 thanks for this! it's looking very promising so far 🎉. One note - we will need to change some of the local development documentation, the Makefile and the docker compose file - which is also used primarily for local development. Since the tests themselves are starting up all of the service dependencies, we won't need the Makefile/docker compose to do it before running tests.

cc/ @fedepaol because you were in the original discussion as well

okay I will address this as well @arschles 👍

ClaytonNorthey92 · 2020-02-20T06:38:44Z

hey @arschles I have removed the dependencies from the testunit service in the docker-compose.yml file, though I don't think I want to remove them from the dev service. I think it is still valid to have them all running locally when developing. plus, when you test, testcontainers will start them mapped to random ports, so they shouldn't clash with any locally running containers

let me know what you think

edit: I think I still need to update the action to fail a "check" if the tests fail in the action...I don't think it is doing that now

arschles · 2020-02-21T20:44:38Z

I have added the workflows directory, based off of your gh-actions branch, here is the latest action running the tests (with testcontainers)

@ClaytonNorthey92 great, I missed that before. We probably need to turn these tests off in drone then. I'd say the best way to do that is with an environment variable that defaults to "on". Then, we can set it to off in drone.

let me know what you think

what you said makes sense for the docker-compose file. I'm glad that things won't conflict!

edit: I think I still need to update the action to fail a "check" if the tests fail in the action...I don't think it is doing that now

I was thinking the same thing. When we merge this PR, tests will probably start failing in GH actions because there are some tokens missing that we need for connecting to various cloud services in our tests. I'll be sure to put those into GH actions secrets 👍

ClaytonNorthey92 · 2020-02-22T14:01:08Z

I have added the workflows directory, based off of your gh-actions branch, here is the latest action running the tests (with testcontainers)

@ClaytonNorthey92 great, I missed that before. We probably need to turn these tests off in drone then. I'd say the best way to do that is with an environment variable that defaults to "on". Then, we can set it to off in drone.

let me know what you think

what you said makes sense for the docker-compose file. I'm glad that things won't conflict!

edit: I think I still need to update the action to fail a "check" if the tests fail in the action...I don't think it is doing that now

I was thinking the same thing. When we merge this PR, tests will probably start failing in GH actions because there are some tokens missing that we need for connecting to various cloud services in our tests. I'll be sure to put those into GH actions secrets 👍

Hey @arschles thanks for the feedback 👍 I should have time to get back to this later this weekend, just wanted to let you know I saw your comments

ClaytonNorthey92 · 2020-02-23T03:15:16Z

hey @arschles
If you look in the "checks" section, I have added the tests action to be run on pull_requests in addition to push, so if the tests fail, it will fail the check.

I believe that these are the environment variables we need for cloud services tests?

GCS_SERVICE_ACCOUNT
GOOGLE_CLOUD_PROJECT
ATHENS_AZURE_ACCOUNT_NAME
ATHENS_AZURE_ACCOUNT_KEY

I notice that in the tests, they are skipped if we do not have the environment variable needed to run them. Would you be able to add these environment variables to Github actions? Then we can re-run the checks and those tests should run and we can ensure that they pass.

I have also removed testing from the .drone.yml file, so all tests are run in Github actions now. I think I would prefer adding those env variables to actions now, rather than the on/off environment variable in drone and test before merging. That seems less invasive. What do you think?

ClaytonNorthey92 · 2020-02-23T03:16:40Z

docker-compose.yml

@@ -22,15 +22,12 @@ services:
      args:
        GOLANG_VERSION: 1.13
    command: ["./scripts/test_unit.sh"]
+    volumes:
+      - /var/run/docker.sock:/var/run/docker.sock # test this on windows, it might be different


@arschles do you have a Windows machine? Could you ensure that the testunit service works on Windows via the make command to run unit tests? If it fails, could you paste the error here?

I believe it is make test-unit-docker

I can try, but let's try to get testcontainers-less appveyor tests back 😄

arschles

Hey @ClaytonNorthey92, let me respond one by one:

Would you be able to add these environment variables to Github actions?

Absolutely. I will do so tomorrow.

What do you think?

About the drone tests, yea it seems like we need to move to GH actions to get this to work. This PR can close #1463 when it gets merged then

arschles · 2020-02-26T01:22:50Z

appveyor.yml

-
-test_script:
-  - go test ./...
-


@ClaytonNorthey92 we still need to run at least unit tests on windows. One idea would be to add an env var to turn off the testcontainers tests and set it in appveyor but not in GH actions. What do you think?

so the issue here came from trying to start the reaper containers on windows (the containers that run and ensure that the containers we start get killed at the end of the tests), but you can optionally skip the reaper containers.

I think that since, according to appveyor's documentation, they don't preserve any VMs between builds, we can safely skip the reaper in appveyor tests. So I think we should add that env variable like TESTCONTAINERS_SKIP_REAPER, if that is set to 1 then we skip the reaper builds and we should be okay (I think)

I am testing this theory out now...I accidentally got a build stuck debugging...so I am going to wait for that to timeout and fail to keep on testing

hey @arschles how do you feel about putting integration tests into their own package? the using a build tag to ignore them by default, then you can pass in the tag to enable them? that should be able to be done just from the code and the config files and wouldn't require a change to the environment

Fine with me RE the different package. I'd rather have them enabled by default and pass a tag to disable them. Are you cool with that?

yeah that makes sense to me, just need a way to cleanly include/not include them

I can work on this

That's great! We had an idea in #1500 to not turn off the tests completely, but just to turn off the testcontainers logic based on a tag. I laid it out here

What do you think about that?

just left a comment

arschles · 2020-02-26T01:23:42Z

.github/workflows/integration-tests.yml

+      run: go build cmd/proxy/*.go
+      env:
+        GOPROXY: https://proxy.golang.org
+        GO111MODULE: "on"


@ClaytonNorthey92 can you add a newline (with no indent) at the end of this file?

just did 👍

arschles · 2020-02-26T01:24:57Z

docker-compose.yml

@@ -22,15 +22,12 @@ services:
      args:
        GOLANG_VERSION: 1.13
    command: ["./scripts/test_unit.sh"]
+    volumes:
+      - /var/run/docker.sock:/var/run/docker.sock # test this on windows, it might be different


I can try, but let's try to get testcontainers-less appveyor tests back 😄

arschles · 2020-02-28T18:39:39Z

@ClaytonNorthey92 I haven't forgotten about this. Things got busy this week. I can set up the secrets in GH actions early next week. I hope that's ok! 😄

ClaytonNorthey92 · 2020-02-28T20:47:19Z

@ClaytonNorthey92 I haven't forgotten about this. Things got busy this week. I can set up the secrets in GH actions early next week. I hope that's ok! 😄

@arschles yes of course, I see there are a few other comments here, I was going to address them this weekend, I have been a little too busy to look this week, but I will this weekend

arschles · 2020-03-07T00:36:26Z

@ClaytonNorthey92 Ok, all the GH Actions secrets are set up! Sorry for that delay 😄

One problem is that the secrets are not passed to PRs from a fork, for security's sake. The best option I can think of is to check for those env vars in the tests and skip if they're not there.

What do you think?

ClaytonNorthey92 · 2020-03-13T16:22:54Z

One problem is that the secrets are not passed to PRs from a fork, for security's sake. The best option I can think of is to check for those env vars in the tests and skip if they're not there.

yeah that makes sense to me, I will make sure that I log out a warning when we skip though, sorry I haven't had time to look back at this, I will have time Saturday

ClaytonNorthey92 · 2020-03-15T17:10:35Z

hey @arschles I started work on moving integration tests over to their own package...I am starting to not like the idea so much. The integration tests where they are use a lot of private attributes to test. For example, the mongo storage, the attribute db is private and are used in testing. So in order to move integration tests to their own package, I would have to either: make these public or expose them via public methods. I have two concerns with this: they would be major changes and I don't know if there are any security risks involved with this. So I propose we include integration tests based on an environment variable, and each test will check if that variable is set, if not, log out a warning and skip the test.

what do you think?

arschles · 2020-03-23T19:48:18Z

So I propose we include integration tests based on an environment variable, and each test will check if that variable is set, if not, log out a warning and skip the test.

@ClaytonNorthey92 that sounds great. Except on the last part - if the variable is not set, can it just continue with the test and assume the service is already running?

ClaytonNorthey92 · 2020-03-24T03:26:51Z

So I propose we include integration tests based on an environment variable, and each test will check if that variable is set, if not, log out a warning and skip the test.

@ClaytonNorthey92 that sounds great. Except on the last part - if the variable is not set, can it just continue with the test and assume the service is already running?

@arschles Yeah I can do that. I would also like to make their connection configurations (hosts, ports, etc) configurable via env variables. So then you could run the dependencies however you wanted (docker, just running the process locally, etc.) and set those environment variables so your test could connect to them, however you have them running.

arschles · 2020-03-26T20:24:20Z

@ClaytonNorthey92 sounds great! We'll be around when you're ready 😄 ✌️

Prototype for using testcontainers for dependencies: mongodb, minio, and redis squash me: use testcontainers in appveyor use newer vs attempt docker upgrade

ClaytonNorthey92 · 2020-12-26T17:17:47Z

hey @arschles I apologize I somewhat abandoned this pull request, ultimately, I couldn't find a great way past this build error when compiling the project on windows, the issue has since been fixed and I opened a pull request to testcontainers-go to ensure they don't use a version that can't compile on windows. This should fix the compilation issue that I mentioned, since that dependency is coming from the testcontainers-go project.

However, even if that gets merged and this PR works and gets merged, I don't think I would have time to support this after it is merged. If you believe testcontainers-go is still worth pursuing I hope my work here will help jump-start whoever picks it up next. If I can help at all, please reach out 👍

arschles · 2021-04-12T20:33:59Z

@ClaytonNorthey92 thanks for your reply here - and no worries at all! 😄

ClaytonNorthey92 commented Feb 16, 2020

View reviewed changes

e2etests/all_test.go Outdated Show resolved Hide resolved

ClaytonNorthey92 force-pushed the chore/prototype-testcontainers branch 4 times, most recently from f1ee7ed to 2dacc53 Compare February 17, 2020 03:01

ClaytonNorthey92 commented Feb 17, 2020

View reviewed changes

.drone.yml Outdated Show resolved Hide resolved

ClaytonNorthey92 mentioned this pull request Feb 17, 2020

Is it worth using testcontainers to set up our unit, integration or e2e tests? #1500

Open

ClaytonNorthey92 commented Feb 17, 2020

View reviewed changes

appveyor.yml Outdated Show resolved Hide resolved

arschles requested changes Feb 20, 2020

View reviewed changes

.drone.yml Outdated Show resolved Hide resolved

appveyor.yml Outdated Show resolved Hide resolved

ClaytonNorthey92 force-pushed the chore/prototype-testcontainers branch 2 times, most recently from f2085d0 to f9e556a Compare February 20, 2020 05:56

ClaytonNorthey92 force-pushed the chore/prototype-testcontainers branch 2 times, most recently from 05db8c1 to 31a7938 Compare February 20, 2020 06:32

ClaytonNorthey92 force-pushed the chore/prototype-testcontainers branch 2 times, most recently from fbe5408 to 57947f1 Compare February 23, 2020 02:50

ClaytonNorthey92 commented Feb 23, 2020

View reviewed changes

arschles requested changes Feb 26, 2020

View reviewed changes

ClaytonNorthey92 force-pushed the chore/prototype-testcontainers branch 4 times, most recently from 48e6f0c to 6b90f8e Compare March 3, 2020 04:31

ClaytonNorthey92 force-pushed the chore/prototype-testcontainers branch 3 times, most recently from 8e4ab29 to 3077638 Compare March 3, 2020 05:06

ClaytonNorthey92 force-pushed the chore/prototype-testcontainers branch 2 times, most recently from 307674b to 7a7c3d4 Compare May 16, 2020 19:40

arschles changed the base branch from master to main June 15, 2020 19:21

ClaytonNorthey92 force-pushed the chore/prototype-testcontainers branch from 3f6822a to abd2732 Compare November 30, 2020 05:03

\gomods#1500 - prototype for using testcontainers-go for dependencies

20cd8b1

Prototype for using testcontainers for dependencies: mongodb, minio, and redis squash me: use testcontainers in appveyor use newer vs attempt docker upgrade

ClaytonNorthey92 force-pushed the chore/prototype-testcontainers branch from abd2732 to 20cd8b1 Compare November 30, 2020 05:05

ClaytonNorthey92 closed this Dec 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

\#1500 - prototype for using testcontainers-go for dependencies #1544

\#1500 - prototype for using testcontainers-go for dependencies #1544

ClaytonNorthey92 commented Feb 16, 2020 •

edited by arschles

Loading

bsideup commented Feb 17, 2020

arschles left a comment

ClaytonNorthey92 commented Feb 20, 2020

ClaytonNorthey92 commented Feb 20, 2020

ClaytonNorthey92 commented Feb 20, 2020 •

edited

Loading

arschles commented Feb 21, 2020

ClaytonNorthey92 commented Feb 22, 2020

ClaytonNorthey92 commented Feb 23, 2020

ClaytonNorthey92 Feb 23, 2020

ClaytonNorthey92 Feb 23, 2020

arschles Feb 26, 2020

arschles left a comment

arschles Feb 26, 2020

ClaytonNorthey92 Mar 2, 2020 •

edited

Loading

ClaytonNorthey92 Mar 2, 2020

ClaytonNorthey92 Mar 3, 2020

arschles Mar 7, 2020

ClaytonNorthey92 Mar 13, 2020

arschles Mar 13, 2020

ClaytonNorthey92 Mar 15, 2020

arschles Feb 26, 2020

ClaytonNorthey92 Mar 2, 2020

arschles Feb 26, 2020

arschles commented Feb 28, 2020

ClaytonNorthey92 commented Feb 28, 2020

arschles commented Mar 7, 2020

ClaytonNorthey92 commented Mar 13, 2020

ClaytonNorthey92 commented Mar 15, 2020

arschles commented Mar 23, 2020

ClaytonNorthey92 commented Mar 24, 2020 •

edited

Loading

arschles commented Mar 26, 2020

ClaytonNorthey92 commented Dec 26, 2020

arschles commented Apr 12, 2021

\#1500 - prototype for using testcontainers-go for dependencies #1544

\#1500 - prototype for using testcontainers-go for dependencies #1544

Conversation

ClaytonNorthey92 commented Feb 16, 2020 • edited by arschles Loading

bsideup commented Feb 17, 2020

arschles left a comment

Choose a reason for hiding this comment

ClaytonNorthey92 commented Feb 20, 2020

ClaytonNorthey92 commented Feb 20, 2020

ClaytonNorthey92 commented Feb 20, 2020 • edited Loading

arschles commented Feb 21, 2020

ClaytonNorthey92 commented Feb 22, 2020

ClaytonNorthey92 commented Feb 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arschles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ClaytonNorthey92 Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arschles commented Feb 28, 2020

ClaytonNorthey92 commented Feb 28, 2020

arschles commented Mar 7, 2020

ClaytonNorthey92 commented Mar 13, 2020

ClaytonNorthey92 commented Mar 15, 2020

arschles commented Mar 23, 2020

ClaytonNorthey92 commented Mar 24, 2020 • edited Loading

arschles commented Mar 26, 2020

ClaytonNorthey92 commented Dec 26, 2020

arschles commented Apr 12, 2021

ClaytonNorthey92 commented Feb 16, 2020 •

edited by arschles

Loading

ClaytonNorthey92 commented Feb 20, 2020 •

edited

Loading

ClaytonNorthey92 Mar 2, 2020 •

edited

Loading

ClaytonNorthey92 commented Mar 24, 2020 •

edited

Loading