fix(report): treat threshold as satisfied when efficacy/coverage equals it#280
Open
matuszeg wants to merge 2 commits into
Open
fix(report): treat threshold as satisfied when efficacy/coverage equals it#280matuszeg wants to merge 2 commits into
matuszeg wants to merge 2 commits into
Conversation
…ls it The efficacy and mutant-coverage threshold checks used `<=`, which made a configured threshold of N unsatisfiable when the actual value was exactly N. Most notably, `--threshold-efficacy 100` could never be met even when every reached mutant was killed (100 <= 100 was true, so gremlins exited 10). Switch both comparisons to `<` so a configured threshold of N is satisfied by an actual value of N. The pre-existing "no error" branch of the assessment test never asserted err == nil, which is why this regression went unnoticed; that branch is fixed and boundary cases (== and >) are now covered explicitly for both float64 and int configuration values. Docs updated to spell out the >= semantics.
Reflow the prose added in the previous commit to fit the docs' 80-column line-length limit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
The efficacy and mutant-coverage threshold checks in
internal/report/report.goused
<=, which makes a configured threshold of N% unsatisfiable when theactual value is exactly N%.
The clearest manifestation:
--threshold-efficacy 100can never be met, evenwhen every reached mutant is killed —
100 <= 100is true, so Gremlins exitswith code 10 and reports "below efficacy-threshold". The same bug existed for
--threshold-mcover.The docs describe these flags as "the percent of KILLED mutants" / "how many
mutants are covered by tests", and users naturally write
100to mean "allreached mutants must be killed". Switching both comparisons to
<makes theflags behave that way: a configured threshold of N is satisfied when the actual
value is greater than or equal to N.
Why the existing tests didn't catch this
The
!tc.expectErrorbranch ofTestAssessmentreturned without everasserting
err == nil. So the upstream test named "efficacy >= efficacy-threshold"(value 50, actual efficacy 50%) was already returning an
ExitErrorunder theold
<=semantics — the assertion just never looked.This PR fixes that branch (
if err != nil { t.Fatalf(...) }) so the boundarycases actually verify the no-error path, then adds explicit
<,==,>, and== 0cases for both efficacy and mutant-coverage, against bothfloat64andintconfiguration values (the assess code reads both via theGet[float64]→Get[int]fallback).Changes
internal/report/report.go:<=→<for both efficacy and mutant-coveragethreshold checks.
internal/report/report_test.go: asserterr == nilon the no-error branch;add
==/>/== 0boundary cases forfloat64andintconfig valueson both thresholds; rename rows so duplicate names get distinct identifiers.
docs/docs/usage/commands/unleash/index.md: clarify that the threshold issatisfied when actual ≥ configured (e.g.
--threshold-efficacy 100is metwhen every reached mutant is killed).
Types of changes
Checklist
make all)Further comments
This is technically a behavior change for anyone who today relies on
--threshold-efficacy Nfailing at exactly N%, but I'd argue that behaviorisn't what the flag's name or documentation imply, and the
100case isclearly a bug (the flag is unreachable). Happy to gate this behind a different
flag name or a deprecation cycle if maintainers prefer — let me know.