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

feat: Setup CI to run tests that require external resources #191

Merged
merged 15 commits into from
Aug 10, 2020

Conversation

reggiemcdonald
Copy link
Contributor

@reggiemcdonald reggiemcdonald commented Aug 4, 2020

PR to update CI for tests that use external resources

This is tested and working.

I would love to get some feedback on this approach as I'm quite new to setting up circleci.
Are there downsides to not using minikube as in the jaeger-operator repo?

Background

The current CI configuration cannot run tests that rely on any external resources such as databases. In last week's SIG, I was referred to the jaeger-operator for guidance on how to approach the CI changes. The jaeger-operator repo runs the integration tests using minikube and an ubuntu VM under multiple github workflows.

Implementation

This method borrows the idea of using workflows, however, it does not use kubernetes and minikube. Instead, each integration test (one for mongo, one for gocql) is run by a job in a new integration workflow in circleci. The jobs are run using a Makefile target that creates the docker containers needed to run the test (therefore not using any clusters).
image.

Changes

  • .circleci/config.yml updated to use workflows
  • A new workflow was added to .circleci/config.yml for the integration tests that use a matrix of Makefile targets to run the tests
  • Makefile targets were created for mongo and gocql
  • A helper function was created to move the check for the integration environment variable into a single place
  • Since mongo and cassandra take some time to fully start, a shell script was included to wait for the containers. I think this can be made more elegant than it is right now using dockerize

Additional tests can be added by:

  1. Modifying Makefile to add a new target
  2. Modifying .circleci/config.yml to include the new target in the target matrix

Fixes: #160

\cc @nlehrer @danherr

@Aneurysm9
Copy link
Member

This looks good to me. The jaeger-operator tests need to use minikube as they're testing a Kubernetes operator and thus need to be run in the context of a cluster. I think the approach you've taken here is fine for our needs.

@reggiemcdonald
Copy link
Contributor Author

reggiemcdonald commented Aug 4, 2020

This looks good to me. The jaeger-operator tests need to use minikube as they're testing a Kubernetes operator and thus need to be run in the context of a cluster. I think the approach you've taken here is fine for our needs.

Thats great! The workflow status isn't getting updated. I think that it might be the result of some circleci settings

Update:

I believe that the pending workflow status is the result of the existing ci workflow being marked as required. This PR renames the old workflow to ci/circleci: build so it will never be reported. I currently do not have the privileges to remove this required check.

@reggiemcdonald reggiemcdonald marked this pull request as ready for review August 5, 2020 16:10
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Overall looks good, I like the approach. Minor cleanup on the bash script is needed.

Do you think it would be possible in the future to only have this run when the gocql or mongo instrumentation contain changes?

.circleci/wait.sh Show resolved Hide resolved
.circleci/wait.sh Outdated Show resolved Hide resolved
.circleci/wait.sh Outdated Show resolved Hide resolved
.circleci/wait.sh Outdated Show resolved Hide resolved
.circleci/wait.sh Outdated Show resolved Hide resolved
@reggiemcdonald
Copy link
Contributor Author

reggiemcdonald commented Aug 5, 2020

Overall looks good, I like the approach. Minor cleanup on the bash script is needed.

Do you think it would be possible in the future to only have this run when the gocql or mongo instrumentation contain changes?

Thanks! I was under the impression that the restore_cache step was used to prevent rerunning tests if there weren't any changes. Am I mistaken in that?

EDIT
I see what you mean now. Yes, I think I may be able to write a script to do that

@MrAlias
Copy link
Contributor

MrAlias commented Aug 5, 2020

Overall looks good, I like the approach. Minor cleanup on the bash script is needed.
Do you think it would be possible in the future to only have this run when the gocql or mongo instrumentation contain changes?

Thanks! I was under the impression that the restore_cache step was used to prevent rerunning tests if there weren't any changes. Am I mistaken in that?

EDIT
I see what you mean now. Yes, I think I may be able to write a script to do that

@reggiemcdonald if you think implementing that kind of selectivity will be a large amount of work I'm in favor of merging this and having that work be a subsequent PR. I'll let you be the one to decide on how you want to split it up.

@reggiemcdonald
Copy link
Contributor Author

Overall looks good, I like the approach. Minor cleanup on the bash script is needed.
Do you think it would be possible in the future to only have this run when the gocql or mongo instrumentation contain changes?

Thanks! I was under the impression that the restore_cache step was used to prevent rerunning tests if there weren't any changes. Am I mistaken in that?
EDIT
I see what you mean now. Yes, I think I may be able to write a script to do that

@reggiemcdonald if you think implementing that kind of selectivity will be a large amount of work I'm in favor of merging this and having that work be a subsequent PR. I'll let you be the one to decide on how you want to split it up.

I think I came up with a reasonable solution:

Essentially, the makefile targets run a git diff against master and greps for folder names (e.g. gocql). I chose this because it's unknown how many commits a PR may have. So I chose to diff against master and run the integration tests if the module was modified when compared to master. This ensures that integration tests are run for each commit in a given PR that has a modification to the given module (as supposed to just the single commit that modified the module).

Let me know if you like this change. Happy to revert the change or modify the approach 🙂

Copy link
Contributor

@MrAlias MrAlias 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 for the contribution!

@MrAlias MrAlias added area: testing Related to package testing release:required-for-ga labels Aug 7, 2020
@reggiemcdonald
Copy link
Contributor Author

LGTM. Thank you for the contribution!

My pleasure! You'll need to change the required ci check from integration to the other workflows since I dont have access to the repos circleci admin page. Instructions are here

@MrAlias
Copy link
Contributor

MrAlias commented Aug 7, 2020

LGTM. Thank you for the contribution!

My pleasure! You'll need to change the required ci check from integration to the other workflows since I dont have access to the repos circleci admin page. Instructions are here

@lizthegrey it looks like I don't have access to the needed "branches" section of this repos settings. Is this something you could help with? Giving me access or updating the check that is?

@lizthegrey
Copy link
Member

Affirm, I can flip this around.

@lizthegrey lizthegrey merged commit c318cd3 into open-telemetry:master Aug 10, 2020
@MrAlias MrAlias mentioned this pull request Aug 13, 2020
plantfansam referenced this pull request in plantfansam/opentelemetry-go-contrib Mar 18, 2022
* Provide conformance tests for tracers

The test harness may be used to ensure that a given tracer behaves
according to the expectations set by the API.

* Add `ToMatchError` matcher

* Use DeepEqual to compare unknown types in matchers

Unlike basic `==`/`!=`, `reflect.DeepEqual` can compare arbitrary
types (e.g., `[]string` to `[]string`)

* Use struct instead of string for test context key
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: testing Related to package testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update CI to support integration tests
5 participants