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

proposal: testing: add magic //go:cover ignore comment #53271

Open
carlmjohnson opened this issue Jun 7, 2022 · 17 comments
Open

proposal: testing: add magic //go:cover ignore comment #53271

carlmjohnson opened this issue Jun 7, 2022 · 17 comments
Labels
Projects
Milestone

Comments

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Jun 7, 2022

Problem

The Go coverage tool is very useful, but there's no way to mark a line as not needing coverage. It is common to add panic("unreachable") in place where one expects code never to run unless there is a programmatic error. These lines show up in code coverage output as untested and must be manually inspected in order to be noted that they can be ignored and don't need to be tested.

Proposal

If a line is prefaced by //go:cover ignore the following expression or block will be marked as "ignored".

// some switch ...
case 1:
  return "1"
//go:cover ignore
default: // this line and the next will be marked as ignored
   panic("unreachable")
// ...
$ go test -coverprofile=coverage.out
PASS
coverage: 42% of statements (2% ignored)
ok      size    0.030s

In the HTML output of the cover tool, ignored lines should be marked with yellow. If an ignored line is covered during a test, it should be reported as "ignored but covered" and marked in purple for investigation of whether a programmatic error has taken place.

Cf #31280

@gopherbot gopherbot added this to the Proposal milestone Jun 7, 2022
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Jun 7, 2022

same as #34639

@randall77
Copy link
Contributor

@randall77 randall77 commented Jun 7, 2022

See also #12504

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Jun 7, 2022

Based on prior comments, Rob Pike is opposed to magic comments to control the cover tool. That makes sense, but if there's no knob for controlling the cover tool, it would be good if the tool had some way of detecting and ignoring panic("unreachable") lines on its own. Maybe panic("unreachable") should be its own magic comment?

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jun 7, 2022
@Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Jun 8, 2022

Maybe panic("unreachable") should be its own magic comment?

The discoverability of that feature for new users sounds really low.

I think, a builtin (or somewhere in the std):

func unreachable() {
  panic("unreachable")
}

And the magic comment being unreachable() is better.

This is a feature other modern languages integrates, for example see in rust or GCC & Clang extensions.

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Jun 8, 2022

See #30582 for an unreachable in the standard library.

@breml
Copy link
Contributor

@breml breml commented Jun 12, 2022

I understand, that Rob Pike has a strong voice, but what is about the community?

As mentioned in #31280 (comment), a substantial part of the community already adopted tools, that work with magic comments to handle false positives. The //go:cover ignore comment would do a similar thing and I would not be surprised, if the same parts of the community would adopt this feature.

If such a magic comment is introduced, nobody is required to use it and the Go team can forbid to use it in the Go standard library, the Go tooling, etc. if they really wish to do this.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 12, 2022

If we're never going to use them in the Go standard library and tooling, then it doesn't seem too hard to me to wrap the Go coverage tool with a wrapper that looks for the comments itself.

If we add direct support in the Go coverage tool, we will be regularly rejecting patches to add them to the Go standard library.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 15, 2022

I'm not sure why this has to be part of cover. If you have something that reports the uncovered lines, couldn't it filter them out instead? In general we're thinking about redoing the way cover works to be more inside the compiler, and I'd rather not have the compiler need to introduce per-basic-block magic comments as a concept.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 15, 2022

I wrote this tool a while back: https://pkg.go.dev/rsc.io/tmp/uncover.
You could imagine doing something similar and filtering in that tool instead.
The only time this would need to be in cover is to affect the reported percentage.
Maybe we should remove the reported percentages because people focus too much on them?

@Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Jun 15, 2022

See #30582 for an unreachable in the standard library.

This seems to be a different proposal.
It seems to care about performance and this would be a compiler hint, with an unsafe mode that let the compiler prune unreachable branches.

What I propose is functionally equivalement to panic("unreachable") except the coverage tool would understand it (not like the Gcc builtin then, I've removed that part).

@rsc rsc moved this from Incoming to Active in Proposals Jun 15, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Jun 15, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

@rsc rsc commented Jun 22, 2022

What I propose is functionally equivalement to panic("unreachable") except the coverage tool would understand it

Why is it important for the coverage tool to "understand it"?
Are people focusing too much on the percentages?

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Jun 22, 2022

I think the conversation is becoming a bit circular. The point isn't the percentage. It's to communicate to other devs/one's future self that a certain line can be ignored when looking at coverage summaries. Otherwise one ends up opening files that don't need to be opened just to see what the 1% uncovered lines are and then immediately closing it again after deciding that yes, the panic is unreachable. It's a waste of time.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 22, 2022

If that's the case, then what if cover "grays out" panic("unreachable")?
Is that sufficient?
That's a lot more palatable to me than //go:cover ignore.

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Jun 22, 2022

In my experience, "coverage ignore" comments are used by developers as a hammer to pass coverage gates in CI (usually not 100%), excluding things they don't know how to test or otherwise don't understand why is only partially covered, or just fudging things when they're 0.5% below the gate.

If an ignored line is covered during a test, it should be reported as "ignored but covered" and marked in purple for investigation of whether a programmatic error has taken place.

That this is even possible makes this, imo, the wrong approach. After all, it's easy to just mark things uncovered and don't write test cases for them.

As someone responsible for developer tooling, ignore comments reduce my trust in the system accurately reflecting reality. I'm sure I'm not the only one having seen services crash with "unreachable" panics, these untested code paths should continue to be reported as such.

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Jun 22, 2022

I'm leaving this issue open, because there are still other users who support it, but I think my preference at this point would be something like #30582 being automatically marked as ignored by coverage. If something screws up and the line does get run, it will trigger a panic, so there's no need for a "purple" accidentally covered code color.

@thanm
Copy link
Contributor

@thanm thanm commented Jun 23, 2022

I'd like to understand the exact semantics of this proposed directive a bit better.

Most existing Go compiler directives (ex: go:noinline, go:noescape, etc) apply to the thing that immediately follows the directive. Is this also the case for the proposed directive? Or do we need some sort of control flow analysis to figure out the set of statements to which it applies?

For example, in this code, does the directive only apply to the statement on line 11, or also to the statement on line 9 as well? I would assume this to be the case, given that our code coverage setup doesn't take into account panic paths.

  func example1(int x) {
    doSomething()          // line 9
    //go:cover ignore      // line 10
    doAnotherThing()       // line 11
    return 42
  }

What about the function below-- if control flow reaches "return 42", this implies that the "if" statement has executed. Do we apply the "no cover" property to the "if" statement? That would be consistent with the previous example (again, assuming that we're not taking into account panic paths). What about the println in the defer statement?

func example2(int x) int {
  defer func() {
    println("himom")
  }()
  if x == 101 {
     launchThePaperAirplane()
  } else {
     orderACuppaCoffee()
  }
  //go:cover ignore
  return 42
}

Where can I place go:nocover directives? Is either of these directives illegal?

//go:nocover
func example3(int x) int {
  x++
  if x < 2 &&
  //go:nocover
  x > 9 {
     launchThePaperAirplane()
  }
  return 0
}

If there are multiple directives, should this be taken into account during control flow analysis? In the function below, both arms of the "if" statement have a //go:nocover directive. This implies that the "if" statement should also not be covered-- should the launchThePaperAirplane() call be also considered nocover?

func example4(int x) int {
  if x == 0 {
    //go:nocover
    x++
  } else {
    //go:nocover
    return x
  }
  return launchThePaperAirplane()
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

9 participants