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/test: Add {T,B}.RawLogf methods #14128

Closed
akalin-keybase opened this issue Jan 28, 2016 · 18 comments
Closed

proposal: testing/test: Add {T,B}.RawLogf methods #14128

akalin-keybase opened this issue Jan 28, 2016 · 18 comments

Comments

@akalin-keybase
Copy link

@akalin-keybase akalin-keybase commented Jan 28, 2016

Currently, {T,B}.Logf calls into c.log, which calls decorate on the argument (which prefixes with the file/line, etc.) before appending it to c.output. A library that wants to override this decoration has to resort to printing out \r as a hack, e.g. https://github.com/stretchr/testify/blob/master/assert/assertions.go#L207 .

What if T and B exposed an additional method RawLogf, which wouldn't call decorate on the argument before appending it to c.output? Then the above hack wouldn't be necessary.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 28, 2016

I don't think we want to grow the testing.TB interface or the testing package API much at this point.

One option that might be less invasive is to introduce a magic Logf value that prevents decoration. E.g.:

     t.Logf("some value = %v", val, testing.NoDecorate)

Where NoDecorate is:

    var NoDecorate = noDecorate{}

type noDecorate struct{}

... so users can't replace it, supports ==, and fits nicely in an interface{} for Logf args.

Loading

@bradfitz bradfitz added this to the Unplanned milestone Jan 28, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 28, 2016

/cc @robpike @adg for proposal.

Loading

@adg
Copy link
Contributor

@adg adg commented Jan 28, 2016

One issue I see with your proposal @bradfitz is that vet would need to be made aware of the special value. Not a show-stopper but it smells funny.

@akalin-keybase does the assertion library dislike the decoration because the file/line would always be in the assertion library?

Loading

@akalin-keybase
Copy link
Author

@akalin-keybase akalin-keybase commented Jan 29, 2016

@adg yeah. I've run into this myself, too, where I want to redirect some logger to use T.Logf, but that logger already comes with file/line info, so I don't want the decoration there.

Loading

@adg
Copy link
Contributor

@adg adg commented Jan 29, 2016

Considering what I wrote above, I think RawLogf and RawLog are better than the suggested magic constant. What do you think @bradfitz?

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 29, 2016

I still don't like it. I'd prefer something extensible that also lends itself towards resolving the report-file/line-N-stack-frames-up issue. (forget its number and on poor connection)

We did make the testing.TB interface have a private method so people can't implement it without embedding a T or B, so we can extend it, but I'd rather not.

Loading

@cespare
Copy link
Contributor

@cespare cespare commented Jan 29, 2016

@bradfitz is probably referring to #4899

Loading

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 30, 2016

Indeed, it'd be nice to solve #4899 in the process of this one, if we solve either.

If we had testing.NoDecorate, you could imagine something similar like testing.FramesUp(2) to change the decoration to point up two frames. Or say combine them... testing.Decorate(-1) and testing.Decorate(2) ... negative one for off, >= 0 for how many frames up.

Loading

@adg
Copy link
Contributor

@adg adg commented Aug 15, 2016

Could the problem that prompted this proposal be solved by #2981?

If so, I suggest closing this issue in favor of that one, which has been accepted.

Loading

@akalin-keybase
Copy link
Author

@akalin-keybase akalin-keybase commented Aug 16, 2016

If I'm reading the proposal right, I don't think so. In particular, it looks like the entire log for a test is a single field in the Result object:

{
    "Package": "example.com/foobar",
    "Result": {
        "Name": "TestBar",
        "State": "PASS",
        "T": 1000000,
        "Log": "some test output\n"
    }
}

So I think that field would look like: "file.go:100 some test output\n..." normally, and "file.go:100\rotherfile.go:200 some test output\n..." for logs that want to override the file/line info. Ideally, it would just be "otherfile.go:200 some test output\n...".

Loading

@mdwhatcott
Copy link

@mdwhatcott mdwhatcott commented Aug 24, 2016

Either solution proposed here so far would be wonderful. I'm the author of GoConvey, gunit, and assertions and have long been frustrated that there wasn't a nice way to squelch/tweak the decorations added by the testing package.

Loading

@adg
Copy link
Contributor

@adg adg commented Sep 26, 2016

The proposal in #2981 is coming along nicely (blocked on me) so I think we should decline this in favor of that one.

Loading

@mdwhatcott
Copy link

@mdwhatcott mdwhatcott commented Sep 27, 2016

@adg - I don't see how the work being done on #2981 resolves this particular issue. Don't get me wrong, I'm really excited to have a -json flag, but this issue is all about providing a way to omit the decoration provided by the testing package in order to have more control over the message conveyed to the user in the test output within the currently running test process.

Loading

@cespare
Copy link
Contributor

@cespare cespare commented Sep 27, 2016

@adg the creator of this issue already explained in #14128 (comment) why #2981 doesn't solve this problem :\

This is about the testing framework controlling output in a way that can only be overridden with hacks and half-measures. This proposal is one specific idea for addressing this issue (which is more thoroughly discussed in #4899).

Loading

@adg
Copy link
Contributor

@adg adg commented Sep 27, 2016

Sorry, I skimmed that comment when I was revising this issue.

But the proposal does cover this use case. Output and test logs are emitted as a stream JSON objects as the tests execute. That should be sufficient to implement any kind of UI layer on top of the JSON output. I'm happy to reopen this proposal if that's not the case.

Loading

@cespare
Copy link
Contributor

@cespare cespare commented Sep 27, 2016

@adg #2981 only helps for this use case if the caller wishes to change from being some functions called from normal Go tests (and invoked with go test) to being a post-processor built on top of go test -json.

Loading

@adg adg reopened this Sep 27, 2016
@mdwhatcott
Copy link

@mdwhatcott mdwhatcott commented Sep 27, 2016

Yeah, it's all about being able to modify the output of go test -v (without -json) in the running process to stdout. That's different than building a tool/ui that reads the JSON stdout of go test -json and interpret/adapt it.

Loading

@adg adg added this to the Proposal milestone Oct 31, 2016
@adg adg removed this from the Unplanned milestone Oct 31, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 31, 2016

We're having another proposal review meeting now. Nobody is excited about this. If people want to reform or recolor the output, they can post-process the output with a wrapper tool, especially with the JSON output. But we don't want everybody to start picking their subjectively favorite colors (literally) for the test output. Consistency is nice.

Sorry, I'm going to close this.

Loading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants