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

Ensure REST API server is shut down after test ends and check for goroutine leaks #2803

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

na--
Copy link
Member

@na-- na-- commented Dec 5, 2022

This depends on #2800

@na-- na-- added this to the v0.43.0 milestone Dec 5, 2022
@github-actions github-actions bot requested review from codebien and mstoykov December 5, 2022 22:59
@na-- na-- force-pushed the init-vus-with-context branch from 3c041d2 to ff2a8e4 Compare December 6, 2022 08:31
@na-- na-- force-pushed the check-for-goroutine-leaks branch from 5f953af to f523e37 Compare December 6, 2022 08:32
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2022

Codecov Report

❗ No coverage uploaded for pull request base (init-vus-with-context@ed39495). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 93c5b6d differs from pull request most recent head 2cb2c74. Consider uploading reports for the commit 2cb2c74 to get more accurate results

@@                   Coverage Diff                    @@
##             init-vus-with-context    #2803   +/-   ##
========================================================
  Coverage                         ?   76.07%           
========================================================
  Files                            ?      213           
  Lines                            ?    16591           
  Branches                         ?        0           
========================================================
  Hits                             ?    12621           
  Misses                           ?     3197           
  Partials                         ?      773           
Flag Coverage Δ
ubuntu 76.01% <0.00%> (?)
windows 75.85% <0.00%> (?)

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

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@na-- na-- force-pushed the init-vus-with-context branch from ff2a8e4 to dbde3c5 Compare December 7, 2022 09:21
@na-- na-- force-pushed the check-for-goroutine-leaks branch from f523e37 to 1766cb7 Compare December 7, 2022 09:35
@na-- na-- changed the title Check for goroutine leaks Ensure REST API server is shut down after test ends and check for goroutine leaks Dec 7, 2022
@na-- na-- force-pushed the init-vus-with-context branch from dbde3c5 to ed39495 Compare December 14, 2022 11:39
@na-- na-- force-pushed the check-for-goroutine-leaks branch from 1766cb7 to 2cb2c74 Compare December 14, 2022 11:48
cmd/run.go Outdated Show resolved Hide resolved
Comment on lines +63 to +71
defer func() {
// TODO: figure out why logrus' `Entry.WriterLevel` goroutine sticks
// around and remove this exception.
opt := goleak.IgnoreTopFunction("io.(*pipe).read")
if err := goleak.Find(opt); err != nil {
fmt.Println(err) //nolint:forbidigo
exitCode = 3
}
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

I help here, I will take a look into it.

Copy link
Member Author

Choose a reason for hiding this comment

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

For future reference, this will be resolved in #2833, which I think we should keep as a separate PR, rather than trying to merge it in this series of other PRs?

cmd/run.go Show resolved Hide resolved
@na-- na-- force-pushed the check-for-goroutine-leaks branch from 2a9644a to eea76b1 Compare January 10, 2023 15:39
@na-- na-- requested review from codebien and imiric January 10, 2023 15:39
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from init-vus-with-context to master January 11, 2023 13:29
@na-- na-- merged commit adb82ff into master Jan 11, 2023
@na-- na-- deleted the check-for-goroutine-leaks branch January 11, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants