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
Closed

testing: T and B should implement -ln methods #12011

cespare opened this issue Aug 4, 2015 · 15 comments

Comments

@cespare
Copy link
Contributor

cespare 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
Copy link
Contributor

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

@cespare
Copy link
Contributor Author

cespare 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
Copy link
Contributor

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
Copy link
Member

dominikh 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
Copy link
Contributor Author

cespare 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
Copy link
Contributor Author

cespare 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
Copy link
Contributor

bradfitz 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
Copy link
Member

dominikh commented Aug 4, 2015

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

@bradfitz
Copy link
Contributor

bradfitz commented Aug 4, 2015

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

@davecheney
Copy link
Contributor

@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
Copy link
Contributor

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

eh ?

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Aug 4, 2015
@adg
Copy link
Contributor

adg 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
Copy link
Contributor

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
Copy link
Contributor

robpike commented Aug 6, 2015

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

@cespare
Copy link
Contributor Author

cespare 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 as completed 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.
Projects
None yet
Development

No branches or pull requests

8 participants