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: T and B should implement -ln methods #12011

Closed
cespare opened this issue Aug 4, 2015 · 15 comments

Comments

Projects
None yet
8 participants
@cespare
Copy link
Contributor

commented Aug 4, 2015

Like fmt and log, testing should provide -ln methods for T and B; that is, Errorln, Fatalln, Logln, and Skipln.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

Why ? Those methods already have an implicit line break appended, how would these new methods improve this ?

@cespare

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2015

Same reasoning as for log.Println, log.Fatalln, etc.

I often write things like

log.Println("Updating foo bar in", baz)

or

log.Fatalln("Error while doing x y z:", err)

but when writing tests, I have to remember to use an alternate form:

t.Fatal("Cannot x y z: ", err)    // need space
t.Fatalf("Cannot x y z: %s", err) // need formatting code
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

I think the latter are clearer, having to remember that the -ln version adds a space between the arguments, as well as trailing \n feels less clear to me.

@dominikh

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

If one agreed with you, this would be "unfortunate". T.Fatal already exists, it cannot be removed or changed, and having an alias is worse than having just one slightly badly named function.

However, there's another point here: T.Fatal doesn't necessarily have to be understood as "print this fatal line". It's much rater a statement about the test (it ended fatally). It's not analogous to fmt or log.

@cespare

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2015

@davecheney that argument seems to apply just as much to log.Println, log.Fatalln, etc. Would you have rather seen those functions removed prior to Go 1, or do you feel that there's some significant difference between log and the T/B methods?

I almost always want a space, so I use log.Println/Fatalln much more than I use log.Print/Fatal; this is why the lack of T.Errorln/Fatalln is noticeable to me.

@cespare

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2015

@dominikh sure, one could view it that way.

I just filed this to register the fact that I've consistently felt that these are missing during a few years of writing Go tests. Even if unintentional, that's the impression I got after using the log and testing packages a bunch.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

We can't extend the TB interface so I have a preference to not add common methods to T and B, so TB doesn't look lacking.

@dominikh

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

@bradfitz TB contains unexported methods, so it should be possible to extend it?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Aug 4, 2015

Oh, pleasant surprise whenever evidence of past foresight is found.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

@davecheney that argument seems to apply just as much to log.Println, log.Fatalln, etc. Would you have rather seen those functions removed prior to Go 1, or do you feel that there's some significant difference between log and the T/B methods?

I don't have a position on those, we cannot change the past, but I don't see the argument for adding extra methods to testing to be compelling.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

Oh, pleasant surprise whenever evidence of past foresight is found.

eh ?

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Aug 4, 2015

@adg

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

@davecheney by "foresight" @bradfitz is referring to the private method in the interface, and its associated comment:

// TB is the interface common to T and B.
type TB interface {
    Error(args ...interface{})
    Errorf(format string, args ...interface{})
    Fail()
    FailNow()
    Failed() bool
    Fatal(args ...interface{})
    Fatalf(format string, args ...interface{})
    Log(args ...interface{})
    Logf(format string, args ...interface{})
    Skip(args ...interface{})
    SkipNow()
    Skipf(format string, args ...interface{})
    Skipped() bool

    // A private method to prevent users implementing the
    // interface and so future additions to it will not
    // violate Go 1 compatibility.
    private()
}
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2015

That's a nice trick.

On Wed, 5 Aug 2015 08:49 Andrew Gerrand notifications@github.com wrote:

@davecheney https://github.com/davecheney by "foresight" @bradfitz
https://github.com/bradfitz is referring to the private method in the
interface, and its associated comment:

// TB is the interface common to T and B.
type TB interface {
Error(args ...interface{})
Errorf(format string, args ...interface{})
Fail()
FailNow()
Failed() bool
Fatal(args ...interface{})
Fatalf(format string, args ...interface{})
Log(args ...interface{})
Logf(format string, args ...interface{})
Skip(args ...interface{})
SkipNow()
Skipf(format string, args ...interface{})
Skipped() bool

// A private method to prevent users implementing the
// interface and so future additions to it will not
// violate Go 1 compatibility.
private()

}


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

@robpike

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2015

For consistency with other printing things, this is probably a good thing to do although barely worthwhile.

@cespare

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2017

I just noticed that t.Fatal and t.Error do insert the spaces between arguments (they call fmt.Sprintln), so this change doesn't make sense. It's still inconsistent with fmt/log, but it's probably too late to change now.

I'm not sure how I didn't notice this before, though. The code in question doesn't look like it has changed since 2011.

@cespare cespare closed this Jan 3, 2017

@golang golang locked and limited conversation to collaborators Jan 3, 2018

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