Use integration tests to generate coverage #1516
Comments
yurishkuro
added
good first issue
|
I see that this issue has been added as a good first issue, I'm new to contributing, how may I help? Please let me know, thanks! |
|
Help is appreciated. What kind of assistance are you looking for? |
|
Perhaps an even better way would be to change the tests from using |
|
Dear @yurishkuro , I'm a first-timer here in Jaeger and as much as I work a bit with it and went through some documentation I'm not yet familiar with the code. This change sounds pretty straightforward. Some questions I had while trying to understand your suggested change:
e.g.: in // Copyright...
//go:build storage-integration-badge
// +build storage-integration-badge
...and in .PHONY: badger-storage-integration-test
badger-storage-integration-test:
bash -c "set -e; set -o pipefail; $(GOTEST) -tags=storage-integration-badge $(STORAGE_PKGS) | $(COLORIZE)"??
you mean in the context of CI? github actions? In github actions I could see that there is a workflow ( .PHONY: mem-and-badger-storage-integration-test
mem-and-badger-storage-integration-test: badger-storage-integration-test grpc-plugin-storage-integration-test
.PHONY: badger-storage-integration-test
badger-storage-integration-test:
STORAGE=badger $(MAKE) storage-integration-test
.PHONY: grpc-plugin-storage-integration-test
grpc-plugin-storage-integration-test:
(cd examples/memstore-plugin/ && go build .)
STORAGE=grpc-plugin $(MAKE) storage-integration-test
I was willing to contribute to |
|
OK I just realize that you addressed many of the issues in the PR2474. Is there something else to be done? maybe the I see you are using |
|
@rbroggi Yes your understanding about the build tags proposal is accurate. Right now we don't use build tags and instead control storage integrations via ENV var, which is weird since the all run all the time. My PR #2474 didn't fix it, just added the other tests (which are fairly light weight) to also run all the time, to at least have them always executed one way or another. |
|
Dear @yurishkuro , I will try to work on that tomorrow (I'm currently based in France so time to go to bed :) ). Thank you for the update and clarifications |
* introduction of build tags [grpc_plugin_storage_integration, badger_storage_integration] for better control over badgerstore_test.go and grpc_test.go which being fairly light-weight can be run along with the main make target `test`
* introduction of build tags [grpc_plugin_storage_integration, badger_storage_integration] for better control over badgerstore_test.go and grpc_test.go which being fairly light-weight can be run along with the main make target `test` Signed-off-by: rbroggi <ro_broggi@hotmail.com>
* introduction of build tags [grpc_plugin_storage_integration, badger_storage_integration] for better control over badgerstore_test.go and grpc_test.go which being fairly light-weight can be run along with the main make target `test` Signed-off-by: rbroggi <ro_broggi@hotmail.com>
- introduce build tags grpc_storage_integration, badger_storage_integration, and memory_storage_integration for better control over badgerstore_test.go and grpc_test.go - run memory storage test as part of make-test, and run badger/grpc in a separate CI workflow Part of Use integration tests to generate coverage #1516 Signed-off-by: rbroggi <ro_broggi@hotmail.com> Signed-off-by: Yuri Shkuro <github@ysh.us> Co-authored-by: Yuri Shkuro <github@ysh.us>
…4964) ## Which problem is this PR solving? - Some of our code is not testable unless we run integration tests, e.g. places that require db connection - But we don't collect coverage from integration tests - Resolves #1516 - Resolves #4965 ## Description of the changes - Add coverage generation to storage tests and upload to codecov - Remove several `.nocover` since their packages are now included in coverage via integration tests anyway. Add corresponding `empty_test.go` to avoid linter error. - Remove `println` from badger & sampling storage integration - Change COLORIZE macro to include `| ...` so that it can be overwritten to nothing for faster std output. When output is piped, even to `| cat -`, the buffering prevents incremental test output, so one cannot easily observe test progress ## How was this change tested? - CI is green - [Comment below by codecov](#4964 (comment)) shows 11 reports (inside expander, Flags table) - The global code coverage dropped -0.82%, which is not surprising since some previously excluded packages are now counted, but not all the code in them is exercised even in integration tests (e.g. kafka/auth/tls.go). However, we can ramp it up in future PRs. One area that seems particularly neglected is calling Close on the storage from integration tests, which we're not doing. --------- Signed-off-by: Yuri Shkuro <github@ysh.us>
The
plugin/storage/integrationtests are usingSTORAGEenv var to decide which tests to run. It makes sense for tests that require external storage, but badger/grpc-plugin are currently not running at all.I propose the following changes:
-coverpkgdirective to generate test coverage for the actual packages, and merge it with overall coverage (codecov.io allows submitting multiple reports from CI job)The text was updated successfully, but these errors were encountered: