report when overall threshold present (or) overrides are present #141
Conversation
vladopajic
left a comment
vladopajic
left a comment
There was a problem hiding this comment.
would it be possible to have two flags HasFileOverrides and HasPackageOverrides ?
pkg/testcoverage/check.go
Outdated
|
|
||
| return AnalyzeResult{ | ||
| Threshold: thr, | ||
| HasOverrides: hasOverrides, |
There was a problem hiding this comment.
compileOverridePathRules does not need to return additional value, instead we can use something like this:
HasOverrides: len(overrideRules) > 0,
I have made the changes for this, please have a look |
vladopajic
left a comment
vladopajic
left a comment
There was a problem hiding this comment.
detectOverrides seems quite good direction, please have a look at comments.
also make sure to check make lint, make test and make check-coverage when you are done
pkg/testcoverage/check.go
Outdated
| if len(cfg.Override) > 0 { | ||
| hasFileOverrides, hasPackageOverrides = detectOverrides(cfg.Override) | ||
| } |
There was a problem hiding this comment.
if len(cfg.Override) > 0 { seems unnecessary, detectOverrides should return correct result for any size of cfg.Override
pkg/testcoverage/check.go
Outdated
| if strings.HasSuffix(override.Path, ".go") { | ||
| hasFileOverrides = true | ||
| } | ||
| if strings.HasPrefix(override.Path, "^") { | ||
| hasPackageOverrides = true | ||
| } |
There was a problem hiding this comment.
would it make sense to have these if statements like this:
if is file override {
...
} else {
if it is not file override then it must be package override
}
also file overrides may have .go$ suffix, for example here .
There was a problem hiding this comment.
I updated the check here to
if override name contains ".go" suffix || ".go$" suffix {
// file override
} else {
// pkg override
}
pkg/testcoverage/check.go
Outdated
| if strings.HasPrefix(override.Path, "^") { | ||
| hasPackageOverrides = true | ||
| } | ||
| if hasFileOverrides && hasPackageOverrides { |
There was a problem hiding this comment.
i don't have anything against of merging this if, but it seems unnecessary. these overrides are usually very small, lets say there is 100 of them (99.99% of users will have less then 10). with this size, this for cycle will finish in 1ns or less, so this if wont produce any noticeably benefits. and on another hand because this repo requires 100% coverage, this if will required to have additional tests case that needs to be added or ignored.
|
I checked coverage stats are |
|
@subhambhardwaj thanks for the effort! i'll make new release later today |
3 participants
Changes:
Allow reports of pkg and file level overrides even when overall pkg and file thresholds are 0
Closes: #140