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

Tooling for integration tests #321

Merged
merged 1 commit into from
Jun 17, 2020
Merged

Tooling for integration tests #321

merged 1 commit into from
Jun 17, 2020

Conversation

jrcamp
Copy link
Contributor

@jrcamp jrcamp commented Jun 12, 2020

This adds make and circleci config needed for running separate integration
tests. These tests are intended to still be run from go test so that they can
output coverage data but they may be longer running and talk to external
services like docker.

Note: this is pulling out the build changes from #306 to breakup the PRs.

@jrcamp jrcamp marked this pull request as ready for review June 12, 2020 21:55
@jrcamp jrcamp requested a review from a team June 12, 2020 21:55
@jrcamp jrcamp self-assigned this Jun 12, 2020
@jrcamp
Copy link
Contributor Author

jrcamp commented Jun 12, 2020

@jchengsfx

@jrcamp jrcamp added the WIP label Jun 12, 2020
@jrcamp jrcamp closed this Jun 12, 2020
@jrcamp jrcamp reopened this Jun 12, 2020
@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #321 into master will decrease coverage by 0.81%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #321      +/-   ##
==========================================
- Coverage   83.38%   82.57%   -0.82%     
==========================================
  Files         167      170       +3     
  Lines        8943     9031      +88     
==========================================
  Hits         7457     7457              
- Misses       1161     1249      +88     
  Partials      325      325              
Flag Coverage Δ
#integration 0.00% <ø> (?)
#unit 82.57% <ø> (?)
Impacted Files Coverage Δ
cmd/otelcontribcol/main.go 0.00% <0.00%> (ø)
internal/version/version.go 0.00% <0.00%> (ø)
cmd/otelcontribcol/components.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb6c8c2...4364f9c. Read the comment docs.

@jrcamp jrcamp requested a review from owais June 12, 2020 22:50
.PHONY: precommit
precommit:
$(MAKE) gotidy
$(MAKE) ci
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a target called ci so I removed this. However, it would be nice to have a pre-commit hook, maybe using pre-commit.

@jrcamp jrcamp removed the WIP label Jun 12, 2020
setup:
steps:
- checkout
- restore_module_cache
Copy link
Contributor

Choose a reason for hiding this comment

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

save_module_cache is not executed anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's getting addressed in #310.

- integration-tests:
filters:
tags:
only: /.*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean it'll run on all branches and all tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied from unit tests. But yes looks like that's the case.

@@ -294,3 +317,24 @@ jobs:
"body": "Link to failed job: '"${CIRCLE_BUILD_URL}"'."
}'
when: on_fail

integration-tests:
Copy link
Contributor

Choose a reason for hiding this comment

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

Are using the machine executor because we want to be able to run docker? I thought enabling remote docker execution context on docker executor allowed to spawn docker containers from regular executors as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah you can spawn docker containers but you can't connect to them directly. See https://circleci.com/docs/2.0/building-docker-images/#accessing-services

It is not possible to start a service in remote docker and ping it directly from a primary container or to start a primary container that can ping a service in remote docker.

$(GOTEST) $(GOTEST_OPT_WITH_COVERAGE) ./... && \
go tool cover -html=coverage.txt -o coverage.html ); \
done
@$(MAKE) for-all CMD="make do-unit-tests-with-cover"
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Why do we want to run each module separately now?
  • Does this mean we'll need to upload coverage reports from each module to coveralls now?

Copy link
Contributor Author

@jrcamp jrcamp Jun 15, 2020

Choose a reason for hiding this comment

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

  • Why do we want to run each module separately now?

It was already being done separately, it's just that the looping was done in bash inside the makefile vs. calling for-all. Thought it'd be better to use for-all and also have the make target available in common so you can run it on an individual module if desired.

  • Does this mean we'll need to upload coverage reports from each module to coveralls now?

I think that was already the case. It already generated a coverage.txt inside each module which codecov collects at the end of circleci run.

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. For some reason I thought we used to run a single go test ./... command to test the entire repo in a single run.

.PHONY: integration-tests-with-cover
integration-tests-with-cover:
@echo $(INTEGRATION_TEST_MODULES)
@$(MAKE) for-all CMD="make do-integration-tests-with-cover" ALL_MODULES="$(INTEGRATION_TEST_MODULES)"
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 we've added build tag to the integration test below. Do we need to specify modules like this integration tests? Could we run only tests with integration build flag here and skip the same ones in unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean have a way to automatically detect which modules contain a // +build integration directive? It's possible with a regex but seems hacky. I don't know of a simple way to do it and didn't want to spend time to optimize too heavily given it's still early days for integrations tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think we need to worry about skipping anything for unit tests since pretty much all modules should have unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, you are already doing what I suggested (using --tags). The bash/make code is hard to read for me as must be obvious by now to you :)

Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

LGTM

@tigrannajaryan
Copy link
Member

Merging #321 into master will decrease coverage by 3.54%.

@jrcamp can you clarify this^^^ ?

This adds make and circleci config needed for running separate integration
tests. These tests are intended to still be run from go test so that they can
output coverage data but they may be longer running and talk to external
services like docker.
@jrcamp
Copy link
Contributor Author

jrcamp commented Jun 17, 2020

@tigrannajaryan not sure, something with codecov was out of sync, rebasing against master seemed to fix it. There's 3 new files listed without coverage because we don't appear to have been running tests for the root module before but now are.

@tigrannajaryan tigrannajaryan merged commit cb734dc into open-telemetry:master Jun 17, 2020
@jrcamp jrcamp deleted the integration branch June 17, 2020 22:15
wyTrivail referenced this pull request in mxiamxia/opentelemetry-collector-contrib Jul 13, 2020
This adds make and circleci config needed for running separate integration
tests. These tests are intended to still be run from go test so that they can
output coverage data but they may be longer running and talk to external
services like docker.
mxiamxia referenced this pull request in mxiamxia/opentelemetry-collector-contrib Jul 22, 2020
@jrcamp jrcamp mentioned this pull request Sep 1, 2020
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
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.

4 participants