Skip to content

Better comments/naming in ExpectPanic()#7

Open
liquidgecka wants to merge 1 commit into
masterfrom
brady/add_expect_panic
Open

Better comments/naming in ExpectPanic()#7
liquidgecka wants to merge 1 commit into
masterfrom
brady/add_expect_panic

Conversation

@liquidgecka

Copy link
Copy Markdown
Owner

This changes the variable name from err to iface and updates the comment
to better address the use cases for this function.

This changes the variable name from err to iface and updates the comment
to better address the use cases for this function.

@philpennock philpennock left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LG. I wonder if it's worth having testlib sentinel values, such as AnyPanic, so if you call:

tt.ExpectPanic(myfunc, testlib.AnyPanic, "myfunc should panic with something (don't care what)")

then the testlib handles it?

Or should it be ExpectPanicPredicate() for that scenario, where the second parameter is a function used to validate the interface, and can just be something which always returns true if someone really doesn't care?

@liquidgecka

Copy link
Copy Markdown
Owner Author

I mean its always possible to make the second argument accept a specific function footprint that does the validation as well. My initial stab at this was pretty simple because its fairly easy to write a panic function anyway..

def func TestX(t *testing.T) {
    T := testlib.NewT(t)
    defer T.Finish()
    defer func() {
        i := recover()
        T.NotEqual(i, nil)
        // T.Equal(i, xyz)
    }()
    // Do stuff
}

I was trying to handle the most common case for panics (assertions for code quality checking) and leaving the complex stuff to the caller since it will be more readable that way. I mean, if your implementing a function anyways the above is likely more readable anyway.

What do you think?

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 9b7d3b5 on brady/add_expect_panic into 561e6b2 on master.

@philpennock

Copy link
Copy Markdown
Collaborator

I think the "don't care what the panic is" sentinel value is probably worth implementing, anything beyond that is YAGNI territory. But approved as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants