Skip to content

Conversation

akagami-harsh
Copy link
Member

@akagami-harsh akagami-harsh commented Dec 17, 2023

Which problem is this PR solving?

Description of the changes

  • added a linter to check implementation of goleak in tests
  • added go leak in some tests

How was this change tested?

Checklist

Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
@akagami-harsh akagami-harsh requested a review from a team as a code owner December 17, 2023 16:00
@akagami-harsh akagami-harsh marked this pull request as draft December 17, 2023 16:03
@akagami-harsh
Copy link
Member Author

akagami-harsh commented Dec 17, 2023

@yurishkuro, i have added goleak to some packages (that do not fail) and added a linter to verify goleak is present in tests. Could you please give me some pointers on whether I'm on the right track?

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.

Thanks, looks good as a direction.

@yurishkuro yurishkuro added the changelog:ci Change related to continuous integration / testing label Dec 17, 2023
@yurishkuro
Copy link
Member

Let's fix the comments but not add any more packages, I prefer to merge smaller PRs.

Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
@akagami-harsh akagami-harsh marked this pull request as ready for review December 18, 2023 12:45
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>
@yurishkuro
Copy link
Member

I simplified the script and moved goleak to lint target.

In the previous run some of the packages failed on goleak, so you need to remove them from this PR.

@yurishkuro
Copy link
Member

we could also add goleak into all empty_test.go files, right now they do come up in the output

@yurishkuro yurishkuro enabled auto-merge (squash) December 18, 2023 17:02
@akagami-harsh
Copy link
Member Author

there are still some packages that do not fail, should i also add them

@yurishkuro
Copy link
Member

let's do that in future PRs

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9f63383) 95.61% compared to head (088a165) 95.62%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5010      +/-   ##
==========================================
+ Coverage   95.61%   95.62%   +0.01%     
==========================================
  Files         319      319              
  Lines       18786    18786              
==========================================
+ Hits        17963    17965       +2     
+ Misses        661      659       -2     
  Partials      162      162              
Flag Coverage Δ
cassandra-3.x 25.63% <ø> (ø)
cassandra-4.x 25.63% <ø> (ø)
elasticsearch-5.x 19.90% <ø> (+0.01%) ⬆️
elasticsearch-6.x 19.90% <ø> (ø)
elasticsearch-7.x 20.04% <ø> (ø)
elasticsearch-8.x 20.13% <ø> (+0.01%) ⬆️
grpc-badger 19.52% <ø> (ø)
kafka 14.12% <ø> (ø)
opensearch-1.x 20.02% <ø> (ø)
opensearch-2.x 20.04% <ø> (ø)
unittests 93.37% <ø> (+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.

@yurishkuro yurishkuro merged commit 74293cd into jaegertracing:main Dec 18, 2023
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.

Enable and enforce goroutine leak checks in tests
2 participants