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

test: Added goleak to TestMain in cmd/collector/app/server package folder #5055

Merged
merged 20 commits into from Jan 2, 2024

Conversation

limistah
Copy link
Contributor

@limistah limistah commented Dec 28, 2023

Which problem is this PR solving?

Description of the changes

  • Added TestMain to package folders
  • Added make goleak into unit test ci

How was this change tested?

  • please run make goleak

Checklist

@limistah limistah requested a review from a team as a code owner December 28, 2023 08:02
Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

I spot-checked a couple packages with added goleak checks and the tests now fail with actual goleaks. The objective of the ticket was not just add the checks and break the CI, but to fix the leaks and only then add the checks,

@yurishkuro
Copy link
Member

btw the test workflow was successful here because of a recently introduced bug -- #5057

@limistah
Copy link
Contributor Author

I spot-checked a couple packages with added goleak checks and the tests now fail with actual goleaks. The objective of the ticket was not just add the checks and break the CI, but to fix the leaks and only then add the checks.

Cool, I'd check in the leaks.

Did a quick run on them and noticed a few of them, some were floated from vendor packages.

I might have to reduce the scope of the fix as the errors returned by the go test path/to/package returned quite a lot of them.

@yurishkuro
Copy link
Member

I might have to reduce the scope of the fix as the errors returned by the go test path/to/package returned quite a lot of them.

this was exactly the recommendation in the ticket.

yurishkuro and others added 5 commits December 30, 2023 08:30
Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
This reverts commit 1f02fb4.

Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
@limistah
Copy link
Contributor Author

I might have to reduce the scope of the fix as the errors returned by the go test path/to/package returned quite a lot of them.

this was exactly the recommendation in the ticket.

Great, I added the goleak fixes for cmd/collector/app/server package.

@limistah limistah changed the title Resolves #5006 - Added goleak to TestMain in package folders Resolves #5006 - Added goleak to TestMain in cmd/collector/app/server package folder Dec 30, 2023
Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
@yurishkuro yurishkuro changed the title Resolves #5006 - Added goleak to TestMain in cmd/collector/app/server package folder test: Added goleak to TestMain in cmd/collector/app/server package folder Dec 30, 2023
Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d489e3a) 95.62% compared to head (41a9583) 95.63%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5055      +/-   ##
==========================================
+ Coverage   95.62%   95.63%   +0.01%     
==========================================
  Files         314      314              
  Lines       18294    18296       +2     
==========================================
+ Hits        17493    17498       +5     
+ Misses        642      640       -2     
+ Partials      159      158       -1     
Flag Coverage Δ
cassandra-3.x 25.59% <0.00%> (-0.01%) ⬇️
cassandra-4.x 25.59% <0.00%> (-0.01%) ⬇️
elasticsearch-5.x 19.86% <0.00%> (-0.03%) ⬇️
elasticsearch-6.x 19.87% <0.00%> (-0.01%) ⬇️
elasticsearch-7.x 19.99% <0.00%> (-0.03%) ⬇️
elasticsearch-8.x 20.10% <0.00%> (-0.01%) ⬇️
grpc-badger 19.49% <0.00%> (-0.01%) ⬇️
kafka 14.10% <0.00%> (-0.01%) ⬇️
opensearch-1.x 20.01% <0.00%> (-0.01%) ⬇️
opensearch-2.x 20.01% <0.00%> (-0.01%) ⬇️
unittests 93.32% <100.00%> (+0.01%) ⬆️

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.

Copy link

github-actions bot commented Jan 2, 2024

Test Results

2 053 tests  +1   2 043 ✅ +1   1m 9s ⏱️ ±0s
  219 suites ±0      10 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit 41a9583. ± Comparison against base commit d489e3a.

♻️ This comment has been updated with latest results.

@yurishkuro yurishkuro added the changelog:test Change that's adding missing tests or correcting existing tests label Jan 2, 2024
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Aleem Isiaka <30846935+limistah@users.noreply.github.com>
Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
@yurishkuro yurishkuro enabled auto-merge (squash) January 2, 2024 20:15
Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
auto-merge was automatically disabled January 2, 2024 20:39

Head branch was pushed to by a user without write access

@limistah
Copy link
Contributor Author

limistah commented Jan 2, 2024

I ran make cover on this commit

I have updated to use require instead of assert, it now has 100% coverage

Which gave the below result for pkg/testutils/leakcheck_test.go
image

Signed-off-by: Aleem Isiaka <aleemisiaka@gmail.com>
@yurishkuro yurishkuro enabled auto-merge (squash) January 2, 2024 20:54
@yurishkuro yurishkuro merged commit e85c8fb into jaegertracing:main Jan 2, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:test Change that's adding missing tests or correcting existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants