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

testing: support unordered output in Examples. #10149

Closed
liquidgecka opened this issue Mar 12, 2015 · 28 comments
Closed

testing: support unordered output in Examples. #10149

liquidgecka opened this issue Mar 12, 2015 · 28 comments
Labels
FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@liquidgecka
Copy link

Ideally tests should support unordered output as well as regular output in testing so that every example that uses maps doesn't need to make an output array and sort it (therefor adding complexity to unrelated example code.) This is very useful when dealing with maps which by design return unordered iteration patterns.

An example of the syntax I am proposing (Or something like that):

func Example() {
    x := map[string]int{"a": 1, "b": 2, "c", 3}
    for key, value := range x {
        fmt.Printf("key=%s value=%d\n", key, value)
    }

    // Unordered output:
    // key=a value=1
    // key=b value=2
    // key=c value=3
}

In this case the output on godoc would show the above, while the test would pass regardless of which order the lines were outputted.

@adg adg changed the title Support unordered output in Examples. testing: support unordered output in Examples. Mar 13, 2015
@adg
Copy link
Contributor

adg commented Mar 13, 2015

This is an interesting idea.

So the testing framework would sort the lines of the output and expected output before comparing?

Can you point toward a real-world use of this that might serve as a motivating example?

@davecheney
Copy link
Contributor

I am uncomfortable about this. Map ordering is undefined, and it's an uphill battle to dispel the myth that newer versions of Go changed the map ordering semantics (they were never guaranteed in first place), so I am not keen on this feature, no matter how well meaning.

@adg
Copy link
Contributor

adg commented Mar 13, 2015

@davecheney this feature seems to specifically call out that some operations will have unordered output, which is exactly the consideration we want developers to keep in mind when using maps.

With that said, I'm not sold on the feature, either. That's why I asked for more info.

@liquidgecka
Copy link
Author

Initially when I started working on this I was thinking about how I would document a package like "shuffle" which works like "sort" except that it shuffles items around randomly rather than sorting them [think python random.shuffle()]. Basically an example of this function with Output is completely impossible to show since by definition the output is randomized. Being able to show the expected output, with a warning that it specifically can be in any order is useful in my opinion.

So the function to test shuffle.Shuffle() could look like this:

func ExampleShuffle() {
    x := []int{1, 2, 3, 4, 5}
    shuffle.ShuffleInt(x)
    for index, value := range x {
        fmt.Printf("index[%d] = %d\n", index, value)
    }

    // Unordered output:
    // 4
    // 2
    // 5
    // 1
    // 3
}

Other use cases are like the one I am specifically working on now. A client I work on makes calls to an external service which returns results in an unspecified order. Ideally I don't want to have to sort the output myself here since every call might end up having the same boiler plate around it, therefor making the examples far more complicated to understand. I suspect that this is common in the Example realm specifically because of things like non-repeatable ordering in maps.

All that being said, I actually love the assertion of lack of assured order. Its awesome because it makes clear the only promise that can be made. It does however make things like "fixed output" testing more complicated. I would love to see changes like this because it embraces the assurance of "unordered" in the example to make it clear what is expected.

[If you want I can list off examples of existing library calls that would be difficult to document using the existing '// Output:' mode.. say os.(*File).Readdir(n int) since the results depend on the underlying file system calls, most the functionality in atomic since it necessitates go routines, and likely a few more I can't think of off the top of my head.]

@adg
Copy link
Contributor

adg commented Mar 13, 2015

I think this is a pretty reasonable feature request. What does @robpike think?

@griesemer
Copy link
Contributor

I like this idea as long as it can be kept simple; and @liquidgecka 's example implies a pretty simple implementation:

  1. unordered results are titled just so: "Unordered output:"
  2. each result element is a single line

Thus, the example checker would have to verify that any runtime output is a permutation of the "unordered output" which is easy enough.

The one question I have: Is this too restrictive for practical use?

@minux
Copy link
Member

minux commented Mar 13, 2015

I want to propose that go test check that different invocations of the
example indeed show different order. (say, invoke the example
at most 10 times, and if all output are the same, the test fails)

This is to prevent this being used when the result is deterministic
but the author is just too lazy to write them in the correct order.
Then when the reader sees "Unordered output:" it knows that the
order is definitely not deterministic.

@liquidgecka
Copy link
Author

@minux My worry with that would be that some conditions might make it hard to ensure two runs will have different ordering. For example, the Readdir() example above might return the same order based on file insertion, or based on file name hashes, etc. This depends on the underlying file system implementation (ext vs tmpfs and such) and therefor two runs with the same process and temp directory would return the same results even though the output is still not order assured. Making the test require the output to change might add complexity and introduce possible false failures that could be confusing to somebody debugging.

@davecheney
Copy link
Contributor

This is correct. Map ordering is "unspecified", not "guarenteed to be
random". Trying to verify that the ordering is random across different runs
would be incorrect.

On Fri, Mar 13, 2015 at 4:24 PM, Brady Catherman notifications@github.com
wrote:

@minux https://github.com/minux My worry with that would be that some
conditions might make it hard to ensure two runs will have different
ordering. For example, the Readdir() example above might return the same
order based on file insertion, or based on file name hashes, etc. This
depends on the underlying file system implementation (ext vs tmpfs and
such) and therefor two runs with the same process and temp directory would
return the same results even though the output is still not order assured.
Making the test require the output to change might add complexity and
introduce possible false failures that could be confusing to somebody
debugging.


Reply to this email directly or view it on GitHub
#10149 (comment).

@minux
Copy link
Member

minux commented Mar 13, 2015 via email

@davecheney
Copy link
Contributor

On Fri, Mar 13, 2015 at 4:33 PM, Minux Ma notifications@github.com wrote:

Then how to prevent the example author to always use "Unordered output:"
when "Output:" is fine?

I don't believe this feature should be added. As you point out, it will be
easily abused and grow into some kind of "oh those silly Go developers, why
do I have to type this magic thing so my examples always pass".

We go to great length to make sure map range order is not the same across
runs, even at the expense of slight performance hit.

I know, and I have said previously that I think this is a mistake. But that
is not up for debate any more.


Reply to this email directly or view it on GitHub
#10149 (comment).

@robpike
Copy link
Contributor

robpike commented Mar 13, 2015

I was going to make gri's suggestion before I saw his, a different prefix that says the output may be unsorted.

"Output" means this is what you get.
"Unsorted output" or "Unordered output" means the output contains these lines, but in arbitrary order.

Seems reasonable and easy, but I do worry a bit about the first step onto a slippery slope. It's possible this is the only step we'll take, though.

So cautiously favorable but not convinced.

@liquidgecka
Copy link
Author

I think my final pitch for it would be this: One of golang's most awesome features is its ability to run Examples as tests. This proposal basically extends those capabilities to cases where the output might not have been predictable before without also adding significant complexity or new knowledge requirements to use it. It also allows an API to more clearly emphasize return results as unordered which can help sway confusion, much in the same way that the changes to map[] forced people to understand the unordered nature of it. While this would be minimal for implementation side, it could really help on the documentation, and testing side of the house.

I love tested/documented code, and anything simple that makes writing tests and documentation in makes it easier to complain about those that don't actually take the time to do it! =)

@adg
Copy link
Contributor

adg commented Mar 16, 2015

On 13 March 2015 at 16:36, Dave Cheney notifications@github.com wrote:

I don't believe this feature should be added. As you point out, it will be
easily abused and grow into some kind of "oh those silly Go developers, why
do I have to type this magic thing so my examples always pass".

If the thing you're demonstrating does emit things in an arbitrary order,
then why wouldn't you use "Unsorted output:" ? Isn't that the correct thing
to do?

I'm having trouble understanding your objection.

@davecheney
Copy link
Contributor

My objection mirrors Minux's. I worry that the temptation to use "Unsorted
output:" will grow copy pasta style in the same way that
runtime.GOMAXPROCS(runtime.NumCPU()) proliferates in Go programs today.

Put simply, I don't see the very rare case of documenting the output of a
function who's element's order is undefined as justification for making the
example test feature more complicated -- and I do believe this is a rare
case.

On Mon, Mar 16, 2015 at 1:41 PM, Andrew Gerrand notifications@github.com
wrote:

On 13 March 2015 at 16:36, Dave Cheney notifications@github.com wrote:

I don't believe this feature should be added. As you point out, it will
be
easily abused and grow into some kind of "oh those silly Go developers,
why
do I have to type this magic thing so my examples always pass".

If the thing you're demonstrating does emit things in an arbitrary order,
then why wouldn't you use "Unsorted output:" ? Isn't that the correct thing
to do?

I'm having trouble understanding your objection.


Reply to this email directly or view it on GitHub
#10149 (comment).

@rsc
Copy link
Contributor

rsc commented Apr 10, 2015

This seems like an okay thing.

@rsc rsc added this to the Go1.5 milestone Apr 10, 2015
@robpike
Copy link
Contributor

robpike commented May 8, 2015

Will have to wait for Go 1.6

@robpike robpike modified the milestones: Go1.6, Go1.5 May 8, 2015
@robpike robpike removed their assignment Sep 25, 2015
@rsc
Copy link
Contributor

rsc commented Oct 24, 2015

Still seems okay to do.

@rsc rsc added the Suggested Issues that may be good for new contributors looking for work to do. label Oct 24, 2015
@rsc rsc modified the milestones: Unplanned, Go1.6 Nov 5, 2015
@extemporalgenome
Copy link
Contributor

I would prefer "Unordered" to "Unsorted" as the term -- "unordered" is broader, and has fewer (mis)implications about the underlying matching algorithm.

@adg
Copy link
Contributor

adg commented Feb 4, 2016

+1 on "Unordered". Typical test output is not "sorted".

@liquidgecka
Copy link
Author

yea unordered was what I used in an example and is the proper term for this so +1 to that wording.

@liquidgecka
Copy link
Author

I took a very, very terrible stab at this here: https://go-review.googlesource.com/19280

Its both incomplete and likely downright awful. Please skewer the author with anything that can be done to make this better. I also didn't get to godoc yet which would be necessary I suspect to making this "real" since otherwise it removes the advantage of the visibility in the documentation.

@gopherbot
Copy link

CL https://golang.org/cl/19280 mentions this issue.

@bradfitz bradfitz modified the milestones: Go1.7, Unplanned Feb 8, 2016
@liquidgecka
Copy link
Author

I am not sure how to re-open this, but its not actually done yet as I need to implement the changes in godoc now that the testing package supports the format and all that.

@adg
Copy link
Contributor

adg commented Mar 9, 2016

Reopened.

@adg adg reopened this Mar 9, 2016
@liquidgecka
Copy link
Author

I wasn't sure how to cross-link from tools to here, but the change for godoc is here: https://go-review.googlesource.com/#/c/20458/

Thanks so much @adg for walking me through this process. =)

@cespare
Copy link
Contributor

cespare commented Apr 13, 2016

Is there still follow-up work remaining? The last thing I can find is a comment from @adg on https://go-review.googlesource.com/#/c/20492/:

In that light, Brady do you want to send a variation of this change that only updates the regexp? That way godoc will still recognize examples with unordered output, but won't rely on the new struct field.

@gopherbot
Copy link

CL https://golang.org/cl/22032 mentions this issue.

@golang golang locked and limited conversation to collaborators Apr 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

No branches or pull requests