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.

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

Jump to bottom

Fixed didPanic to now detect panic(nil). #793

Merged
merged 1 commit into from
Mar 15, 2022
Merged

Fixed didPanic to now detect panic(nil). #793

merged 1 commit into from
Mar 15, 2022

Conversation

RmbRT
Copy link
Contributor

@RmbRT RmbRT commented Jul 26, 2019 •
edited

Previously, the function would not detect panic(nil) calls.
In didPanic, removed the anonymous function call, instead,
added named return values. Added extra test cases for the
panic(nil) call.

Closes #794.

@MariusVanDerWijden
Copy link

Nice!

@RmbRT RmbRT mentioned this pull request Jul 26, 2019
Closed
@sebastianst
Copy link

ACK good catch

sebastianst
sebastianst previously approved these changes Jul 26, 2019
View reviewed changes
@ggwpez
Copy link

ggwpez commented Jul 26, 2019

Hes my brother 💯

boyan-soubachov
boyan-soubachov requested changes Jun 28, 2020
View reviewed changes
Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Choose a reason for hiding this comment

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

Thanks for your effort. I've added some comments, would appreciate your feedback.

Could you also rebase this PR onto the latest master since it has been a while :)

assert/assertions.go

return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this line not redundant/pointless now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's required when returning named return values.

assert/assertions_test.go Outdated
t.Error("didPanic should return true")
const panicMsg = "Panic!"

if funcDidPanic, msg := didPanic(func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be a bit clearer if we separated these if statements into two lines, assignment and if check?

e.g.

funcDidPanic, msg := didPanic(func() {...})
if !funcDidPanic || msg != panicMsg {
    ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's fine as it is. Reads as naturally as any other go code to me.

@Antonboom
Copy link

Any updates? :)

boyan-soubachov
boyan-soubachov previously approved these changes Mar 15, 2022
View reviewed changes
Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution :)

@boyan-soubachov
Copy link
Collaborator

This PR needs to be updated and conflicts resolved before I can merge it.

@RmbRT
Fixed didPanic to now detect panic(nil).
dfbd2d6 
Previously, the function would not detect panic(nil) calls.
In didPanic, removed the anonymous function call, instead,
added named return values. Added extra test cases for the
panic(nil) call.
@RmbRT
Copy link
Contributor Author

RmbRT commented Mar 15, 2022

Rebased, resolved conflicts.

@boyan-soubachov boyan-soubachov merged commit 083ff1c into stretchr:master Mar 15, 2022
tisonkun added a commit to tisonkun/assert that referenced this pull request Mar 23, 2022
@tisonkun @RmbRT
fix: didPanic should detect panic(nil)
c01eb93 
This is cheery-picked from stretchr/testify#793.

Signed-off-by: tison <wander4096@gmail.com>
Co-authored-by: RmbRT <steffen.rattay@gmail.com>
Sign up for free to join this conver