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

Collect code coverage from integration tests and upload to codecov #4964

Merged
merged 13 commits into from
Nov 25, 2023

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Nov 24, 2023

Which problem is this PR solving?

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 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>
@yurishkuro yurishkuro requested a review from a team as a code owner November 24, 2023 15:20
Copy link

codecov bot commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dfd6f63) 96.42% compared to head (a92deba) 95.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4964      +/-   ##
==========================================
- Coverage   96.42%   95.59%   -0.83%     
==========================================
  Files         304      317      +13     
  Lines       18092    18689     +597     
==========================================
+ Hits        17445    17866     +421     
- Misses        513      661     +148     
- Partials      134      162      +28     
Flag Coverage Δ
cassandra-3.x 25.64% <ø> (?)
cassandra-4.x 25.64% <ø> (?)
elasticsearch-5.x 19.90% <ø> (?)
elasticsearch-6.x 19.91% <ø> (?)
elasticsearch-7.x 20.05% <ø> (?)
elasticsearch-8.x 20.12% <ø> (?)
grpc-badger 19.56% <ø> (?)
kafka 14.07% <ø> (?)
opensearch-1.x 20.03% <ø> (?)
opensearch-2.x 20.05% <ø> (?)
unittests 93.34% <ø> (-3.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro added the changelog:ci Change related to continuous integration / testing label Nov 24, 2023
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
.PHONY: token-propagation-integration-test
token-propagation-integration-test:
go clean -testcache
bash -c "set -e; set -o pipefail; $(GOTEST) -tags token_propagation -run TestBearTokenPropagation $(STORAGE_PKGS) | $(COLORIZE)"
Copy link
Member Author

Choose a reason for hiding this comment

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

was not used anywhere

@@ -61,6 +61,7 @@ GOFMT=gofmt
GOFUMPT=gofumpt
FMT_LOG=.fmt.log
IMPORT_LOG=.import.log
COLORIZE ?= | $(SED) 's/PASS/✅ PASS/g' | $(SED) 's/FAIL/❌ FAIL/g' | $(SED) 's/SKIP/☠️ SKIP/g'
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed 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

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro changed the title Merge codecov reports Collect code coverage from integration tests and upload to codecov Nov 24, 2023
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

Nice 👍🏼

@@ -1,8 +1,17 @@
codecov:
notify:
require_ci_to_pass: yes
after_n_builds: 11
Copy link
Contributor

Choose a reason for hiding this comment

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

Why specifically 11?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's how many reports we upload

@yurishkuro yurishkuro merged commit 47f39a8 into jaegertracing:main Nov 25, 2023
36 checks passed
@yurishkuro yurishkuro deleted the codecov-merge-reports branch November 25, 2023 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:ci Change related to continuous integration / testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unused token-propagation-integration-test Use integration tests to generate coverage
2 participants