Skip to content

Make Error and Fatal functions compatible with testing.T #286

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Make Error and Fatal functions compatible with testing.T #286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
52ca7b9
chore: make Error and Fatal functions compatible with testing
sirockin Sep 11, 2024
3d9fb97
Merge branch 'master' into sirockin-tb-compatibility
sirockin Sep 11, 2024
eee6067
test: update Fatal/Error tests to take account of more flexible argum…
sirockin Sep 11, 2024
0d29602
Merge branch 'sirockin-tb-compatibility' of ssh://github.com/sirockin…
sirockin Sep 11, 2024
ac66ef9
test: add t.Parallel() to subtests
sirockin Sep 11, 2024
6fe4614
Merge branch 'master' into sirockin-tb-compatibility
sirockin Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions pkg/f1/testing/t.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,18 @@ func (t *T) Errorf(format string, args ...interface{}) {
t.Fail()
}

// Error is equivalent to Log followed by Fail.
func (t *T) Error(err error) {
t.logger.Error("iteration failed", log.IterationAttr(t.Iteration), log.ErrorAttr(err))
// Error logs an error then calls Fail()
func (t *T) Error(args ...interface{}) {
// Special case, single error argument
if len(args) == 1 {
err, ok := args[0].(error)
if ok {
t.logger.Error("iteration failed", log.IterationAttr(t.Iteration), log.ErrorAttr(err))
t.Fail()
return
}
}
t.Log(args...)
t.Fail()
}

Expand All @@ -158,9 +167,18 @@ func (t *T) Fatalf(format string, args ...interface{}) {
t.FailNow()
}

// Fatal is equivalent to Log followed by FailNow.
func (t *T) Fatal(err error) {
t.logger.Error("iteration failed", log.IterationAttr(t.Iteration), log.ErrorAttr(err))
// Fatal logs an error then calls FailNow.
func (t *T) Fatal(args ...interface{}) {
// Special case, single error argument
if len(args) == 1 {
err, ok := args[0].(error)
if ok {
t.logger.Error("iteration failed", log.IterationAttr(t.Iteration), log.ErrorAttr(err))
t.FailNow()
return
}
}
t.Log(args...)
Comment on lines +172 to +181
Copy link
Contributor

@nvloff-f3 nvloff-f3 Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would have different logging depending on the number of arguments.

I think if we just add a new log Attr helper that merges a list of errors, we can handle any number of arguments in a consistent way

Copy link
Author

@sirockin sirockin Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick review and useful feedback, @nvloff-f3 .

I'm still not exactly clear on what you are looking for though. The original comment said // Error is equivalent to Log followed by Fail. (consistent with behaviour of testing.T) so I was aiming to get as close to that as possible.

Please could you clarify the behaviour you would like to see like to see with:

  • a single argument of type error
  • a single argument of type other than error
  • more than one argument, all of type error
  • more than one argument, not all of type error

It would be great if we could create some tests for these scenarios but given the way that concrete logger instances are embedded into t, that may be difficult to achieve.

It could also be argued that rather than introducing a helper to log the arguments, we should amend Log so that it does what we want then use that in the methods. This would simplify code, make behaviour consistent across these methods and make the original comments for Error and Fatal accurate. However it would mean that we couldn't force logger.Error to be called if standard printf arguments were supplied to Error or Fatal.

Anyway let me know your thoughts and I'll prepare a commit.

Thanks again!

t.FailNow()
}

Expand Down
70 changes: 54 additions & 16 deletions pkg/f1/testing/t_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,30 @@ func TestFailSetsTheFailedState(t *testing.T) {
func TestErrorSetsTheFailedState(t *testing.T) {
t.Parallel()

newT, teardown := newT()
defer teardown()

newT.Error(errors.New("boom"))
require.True(t, newT.Failed())
tests := map[string]struct {
args []any
}{
"error argument": {
args: []any{errors.New("boom")},
},
"no arguments": {
args: []any{},
},
"random arguments": {
args: []any{"boom", 1, 2.0},
},
}

for testName, test := range tests {
t.Run(testName, func(t *testing.T) {
t.Parallel()
newT, teardown := newT()
defer teardown()

newT.Error(test.args...)
require.True(t, newT.Failed())
})
}
}

func TestErrorfSetsTheFailedState(t *testing.T) {
Expand All @@ -132,17 +151,36 @@ func TestErrorfSetsTheFailedState(t *testing.T) {
func TestFatalSetsTheFailedState(t *testing.T) {
t.Parallel()

newT, teardown := newT()
defer teardown()

done := make(chan struct{})
go func() {
defer catchPanics(done)
newT.Fatal(errors.New("boom"))
}()
<-done

require.True(t, newT.Failed())
tests := map[string]struct {
args []any
}{
"error argument": {
args: []any{errors.New("boom")},
},
"no arguments": {
args: []any{},
},
"random arguments": {
args: []any{"boom", 1, 2.0},
},
}

for testName, test := range tests {
t.Run(testName, func(t *testing.T) {
t.Parallel()
newT, teardown := newT()
defer teardown()

done := make(chan struct{})
go func() {
defer catchPanics(done)
newT.Fatal(test.args...)
}()
<-done

require.True(t, newT.Failed())
})
}
}

func TestFatalfSetsTheFailedState(t *testing.T) {
Expand Down