Tested debug sync #2
Conversation
jbsv
force-pushed
the
tested_debug_sync
branch
from
e525b8a
to
24b3008
Compare
2nd attempt to remove useless lint warnings fix make check
jbsv
force-pushed
the
tested_debug_sync
branch
from
24b3008
to
4dbe2ec
Compare
mod.go
Outdated
| // LLVL=trace go test ./... | ||
| // LLVL=info go test ./... | ||
| // DBGSYNCLOG=trace go test ./... | ||
| // DBGSYNCLOG=info go test ./... |
There was a problem hiding this comment.
should we mention the DebugFlag here as well ?
mod.go
Outdated
|
|
||
| const defaultLevel = zerolog.NoLevel | ||
|
|
||
| func init() { | ||
| dbg := os.Getenv(DebugFlag) | ||
| DebugIsOn = dbg == "true" |
There was a problem hiding this comment.
how about a case insensitive comparison?
There was a problem hiding this comment.
It's really just a matter of taste, no ? Maybe we should pick "1" or "0" as possibilities ;-)
mod.go
Outdated
| @@ -16,11 +17,15 @@ import ( | |||
|
|
|||
| // EnvLogLevel is the name of the environment variable to change the logging | |||
| // level. | |||
| const EnvLogLevel = "LLVL" | |||
| const EnvLogLevel = "DBGSYNCLOG" | |||
| const DebugFlag = "DBGSYNCON" | |||
There was a problem hiding this comment.
this constant is missing documentation
rwmutex.go
Outdated
| // when m.wg goes bellow zero, | ||
| // we need to recover from panic for the unit tests, | ||
| // re-init the wg |
There was a problem hiding this comment.
| // when m.wg goes bellow zero, | |
| // we need to recover from panic for the unit tests, | |
| // re-init the wg | |
| // when the WaitGroup goes below zero, | |
| // we need to recover from panic, | |
| // and re-initialize it |
suggestion for a tiny phrasing fix, not sure where the 80 characters limit is 😄
| m.wg.Done() | ||
| defer func() { | ||
| err := recover() | ||
| if err != nil { |
There was a problem hiding this comment.
Could we use errors.Is to verify that the err is indeed because we went below 0, or match a string ?
( I don't remember if recover provides an error or a string, and if it's a string if we have a type to compare it to )
I'm just thinking it would be a pity to "hide" an unrelated panic, somehow.
There was a problem hiding this comment.
"The return value of recover is the error raised in the call to panic." Hard to use a Is(...) in this case. But maybe I could check if the wg goes bellow zero ?
rwmutex_test.go
Outdated
| // There is a modified copy of this file in runtime/rwmutex_test.go. | ||
| // If you make any changes here, see if you should make them there. |
There was a problem hiding this comment.
Huh? I assume this is left over from something older, right?
I don't think anybody maintaining this code is going to make changes to the Go runtime standard library 😁
| // Copyright 2011 The Go Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style | ||
| // license that can be found in the LICENSE file. |
There was a problem hiding this comment.
This seems odd 😁
Shouldn't we add a header mentioning that this is based off the sync standard library tests ?
There was a problem hiding this comment.
I'm very uncomfortable with this licensing stuff and yes, I like your proposition.
| // Copyright 2009 The Go Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style | ||
| // license that can be found in the LICENSE file. | ||
|
|
There was a problem hiding this comment.
This seems odd 😁
Shouldn't we add a header mentioning that this is based off the sync standard library tests ?
| // Copyright 2009 The Go Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style | ||
| // license that can be found in the LICENSE file. |
There was a problem hiding this comment.
This seems odd 😁
Shouldn't we add a header mentioning that this is based off the sync standard library tests ?
|
Kudos, SonarCloud Quality Gate passed! |
DBGSYNCON=trueDBGSYNCLOG=info