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

Replace goleak.VerifyTestMain with testutils.VerifyGoLeaks #5108

Merged
merged 11 commits into from
Jan 16, 2024

Conversation

Pushkarm029
Copy link
Member

@Pushkarm029 Pushkarm029 commented Jan 16, 2024

Which problem is this PR solving?

Description of the changes

  • Fixed Script to count testutils.VerifyGoLeaks as satisfactory.
  • Replaced goleak.VerifyTestMain with testutils.VerifyGoLeaks.

How was this change tested?

  • make test

Checklist

Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>
@Pushkarm029 Pushkarm029 requested a review from a team as a code owner January 16, 2024 05:37
Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>
Copy link

github-actions bot commented Jan 16, 2024

Test Results

2 060 tests  ±0   2 050 ✅ ±0   1m 8s ⏱️ -1s
  216 suites ±0      10 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit b65c702. ± Comparison against base commit 3c9da0d.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3c9da0d) 95.59% compared to head (b65c702) 95.56%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5108      +/-   ##
==========================================
- Coverage   95.59%   95.56%   -0.04%     
==========================================
  Files         317      317              
  Lines       18284    18284              
==========================================
- Hits        17479    17473       -6     
- Misses        647      651       +4     
- Partials      158      160       +2     
Flag Coverage Δ
cassandra-3.x 25.58% <ø> (ø)
cassandra-4.x 25.58% <ø> (ø)
elasticsearch-5.x 19.87% <ø> (ø)
elasticsearch-6.x 19.85% <ø> (ø)
elasticsearch-7.x 19.99% <ø> (-0.02%) ⬇️
elasticsearch-8.x 20.08% <ø> (ø)
grpc-badger 19.51% <ø> (ø)
kafka 14.09% <ø> (ø)
opensearch-1.x 19.99% <ø> (-0.02%) ⬇️
opensearch-2.x 20.00% <ø> (ø)
unittests 93.22% <ø> (-0.04%) ⬇️

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: Pushkar Mishra <pushkarmishra029@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.

you can also add the uber/goleak to disallowed packages in .golangci.yml, similar to disallow-crossdock

cmd/agent/app/processors/thrift_processor_test.go Outdated Show resolved Hide resolved
@@ -11,7 +11,6 @@
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

Copy link
Member

Choose a reason for hiding this comment

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

what's with the changes in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was resolving conflicts but forgot that I needed to remove this change.

@yurishkuro yurishkuro mentioned this pull request Jan 16, 2024
4 tasks
Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>
Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>
Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>
.golangci.yml Outdated Show resolved Hide resolved
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>
.golangci.yml Outdated Show resolved Hide resolved
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>
s2 := &model.Span{}

Copy link
Member

Choose a reason for hiding this comment

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

why these changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because while I was doing these changes, I was two commits behind the main. I was manually resolving conflicts by comparing changes. But by mistake, I might have forgotten.

removing it now

@yurishkuro yurishkuro changed the title Replaced goleak.VerifyTestMain with testutils.VerifyGoLeaks and Fixed Script Replace goleak.VerifyTestMain with testutils.VerifyGoLeaks Jan 16, 2024
Signed-off-by: Pushkar Mishra <pushkarmishra029@gmail.com>
Signed-off-by: Pushkar Mishra <pushkarmishra029@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.

Thanks!

@yurishkuro yurishkuro added the changelog:test Change that's adding missing tests or correcting existing tests label Jan 16, 2024
@yurishkuro yurishkuro enabled auto-merge (squash) January 16, 2024 07:02
@yurishkuro yurishkuro merged commit 663a04e into jaegertracing:main Jan 16, 2024
39 of 40 checks passed
@Pushkarm029 Pushkarm029 deleted the goleak3 branch January 16, 2024 07:33
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.

2 participants