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: remove testable examples #26971

Closed
dsnet opened this issue Aug 13, 2018 · 6 comments

Comments

Projects
None yet
6 participants
@dsnet
Copy link
Member

commented Aug 13, 2018

(I'm increasingly seeing this problem, but I also see a lot of pragmatic benefit to testable examples. Perhaps this issue will spur discussions on how to resolve this in a more scalable way).

The Problem

In the testing package, you are allowed to append a special "Output:" comment at the end of examples such that the example will be run as a test and the output directly compared as strings (ignoring whitespace).

This feature of turning examples into tests has a lot of pragmatic benefits. It's very useful in the strings or any package that has relatively small number of dependencies on other packages. However, this feature breaks down on examples that depend on output from 3rd party packages not under the example author's control, which happens to be a dominant case at scale.

For example authors it is often not obvious that they are even depending on stable output when they write their test. For example, simply writing fmt.Println(v) inherently ties the test to the concrete implementation of v (and nested types in v) that may not be under the author's control.

Suppose v was:

type Foo struct {
    ...
    BarField bar.Value
}

Even though, v may be a type owned by the test author, bar.Value is a type that is owned by some external author (e.g., me). If I add a bar.Value.String method or change an existing String method in a package that I own, I accidentally break any tests that may have been printing my type.

The existence of testable example outputs based on strings encourages test authors to depend on this property, and makes it difficult for library maintainers at scale.

I have seen this problem repeatedly show itself time after time when any of the following packages changes it outputs: compress/*, archive/*, encoding/*, the String method on protocol buffer messages, cmp.Diff, etc.

As a library maintainer at Google, a large proportion of my engineering time is dealing with poorly written tests, and example tests are a significant offender. One of Go's goals are development scale, and example tests are hindering this goal.

Proposed Solution

As much benefit testable examples bring, I believe they bring more harm at scale, and so I propose that that we remove the ability for the testing package. However, continue to build examples.

I would love to hear alternative solutions.

@gopherbot gopherbot added this to the Proposal milestone Aug 13, 2018

@gopherbot gopherbot added the Proposal label Aug 13, 2018

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2018

However, this feature breaks down on examples that depend on output from 3rd party packages not under the example author's control, which happens to be a dominant case at scale.

This seems to be the problem, not the testing package. Is this proposal better expressed as a desire to give guidance to authors for what and what does not consistitute a testable example?

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

This seems to be the problem, not the testing package. Is this proposal better expressed as a desire to give guidance to authors for what and what does not consistitute a testable example?

If we keep testable examples, we should provide better guidance for what does/doesn't make a good testable example.

However, my experience is that it is still very easy to violate the suggestions. I'm aware of these implications myself and I looked at some of my own examples and noticed I'm also depending on 3rd party packages that I'm not an owner of (e.g., through fmt.Println). Thus, it is a problem with the testing package if it too easily encourages poor practices.

@mekegi

This comment has been minimized.

Copy link

commented Aug 14, 2018

@dsnet
It's completely not a problem
For stable and reproducable build, tests - you just need use some dependency management (vgo, glide and etc)
If you set exact version of all you dependencies - than your examples always have stable reproducable output

@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

Writing good documentation is hard. Writing a good test is hard. Therefore, writing a good example is very hard. I don't see why that means we should do away with them.

The guidance should be don't output anything not under the control of the current package.

This seems somewhat amenable to static analysis. Find a fmt.Println(v), look up the type of v, if it relies on printing unexported fields or third party types, emit a warning or error. Not foolproof but it seems like it could catch a lot of common mistakes with some basic heuristics.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

If we are going to have examples, we should have tests to make sure they're correct, right?
Not having examples doesn't seem like a viable option: they're probably the only part of the docs that many people read.

It would be better to focus on ways to make examples easier to write more robustly, whether new mechanisms or new documentation.

Can you give more concrete examples of "examples that depend on output from 3rd party packages not under the example author's control"? I'm having a hard time getting from your general description to a specific problem I can think through.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2018

Closing due to lack of response. If there is a specific problem, let's focus on that problem (in separate issues). We're not going to remove testable examples as a whole feature.

@rsc rsc closed this Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.