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.

Sign up for GitHub

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

Jump to bottom

expvar: RemoveAll is used for tests only #27555

Closed
wfernandes opened this issue Sep 7, 2018 · 3 comments
Closed

expvar: RemoveAll is used for tests only #27555

wfernandes opened this issue Sep 7, 2018 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Go1.12

Comments

@wfernandes
Copy link
Contributor

What version of Go are you using (go version)?

go version go1.11 darwin/amd64

go/src/expvar/expvar_test.go

Lines 21 to 30 in 3e5b5d6

// RemoveAll removes all exported variables.
// This is for tests only.
func RemoveAll() {
varKeysMu.Lock()
defer varKeysMu.Unlock()
for _, k := range varKeys {
vars.Delete(k)
}
varKeys = nil
}

The comment for expvar.RemoveAll() says This is for tests only. So at the very least this function should be made private.

On the other hand, it would be useful to be able to remove a metric and perhaps "Reset" all metric variables. I understand that the underlying sync.Map is optimized for many reads and infrequent writes but we could add godoc to make it clear that the "Reset" or "RemoveAll" would be expensive and should be used infrequently.
@bcmills
Copy link
Contributor

bcmills commented Sep 7, 2018

So at the very least this function should be made private.

It's only defined in the test itself, so nobody outside that test should be ably to use it anyway. Why does it matter whether it's exported?

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 7, 2018
@bcmills bcmills added this to the Go1.12 milestone Sep 7, 2018
@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Sep 7, 2018
@wfernandes
Copy link
Contributor Author

At first I thought that too, but since the test is referencing the global vars variable, the tests are actually in the expvar package and not the expvar_test package.
Maybe I'm misunderstanding something because I don't see it in the godoc as well. Maybe having it defined inside a _test.go file is enough to hide it away.

On the other hand it would be nice to have the ability to reset the vars entirely.

@ianlancetaylor
Copy link
Contributor

A function defined in a x_test.go file is not added to the package, and is not available to code that imports that package. This is true even if the x_test.go is part of the package.

I don't think there is anything to do here, so closing.

@as as mentioned this issue Dec 5, 2018
Open
@golang golang locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
4 participants
@wfernandes @ianlancetaylor @bcmills @gopherbot