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

suite: missing signature check before calling Test* method #1508

Open
wvell opened this issue Nov 16, 2023 · 4 comments · May be fixed by #1512 or #1665
Open

suite: missing signature check before calling Test* method #1508

wvell opened this issue Nov 16, 2023 · 4 comments · May be fixed by #1512 or #1665
Labels
bug help wanted pkg-suite Change related to package testify/suite

Comments

@wvell
Copy link

wvell commented Nov 16, 2023 •
edited
Loading

If a test method on a suite has a parameter there is a panic:

func (suite *SomeTestSuite) TestUpdateSomething(somearg string) {
......
}

When running this you get the following panic:

suite.go:87: test panicked: reflect: Call with too few input arguments
....

I think the error handling could be more clear here. Something like "method TestUpdateSomething on SomeTestSuite contains arguments"

Here is the go.dev link: https://go.dev/play/p/cNNfqH5QpeQ

I can create a pull request if this change is wanted?

Edit: Some more context. I run into this problem when I am porting old test methods to a suite.
@dolmen
Copy link
Collaborator

dolmen commented Nov 19, 2023

Please provide an example running on go.dev/play.

@wvell
Copy link
Author

wvell commented Nov 20, 2023

I've added a go.dev example.

@dolmen
Copy link
Collaborator

dolmen commented Nov 22, 2023

This is definitely a dirty bug: Suite should check the method signature and report a test failure if the type doesn't match.

A PR would be welcome.

@dolmen dolmen added bug pkg-suite Change related to package testify/suite help wanted labels Nov 22, 2023
@dolmen dolmen changed the title test panicked: reflect: Call with too few input arguments suite: missing signature check before calling Test* method Nov 22, 2023
@Barugoo Barugoo linked a pull request Nov 30, 2023 that will close this issue
Open
@vyas-git
Copy link

vyas-git commented Oct 20, 2024 •
edited
Loading

Hi, I would like to take this up, so please assign me to the issue.

I’ve noticed the PR linked to this issue, but I’m not sure if it’s still active. I have raised a new PR with the required improvements. I agree with @dolmen's point that we should check for invalid method signatures earlier in the flow, after the methodFilter.

Additionally, I used t.Errorf instead of t.Fatalf to report invalid method signatures while continuing the execution of valid methods. The PR also includes additional test scenarios covering both valid and invalid method signatures.

You can find the new PR here.

@brackendawson @dolmen @arjunmahishi Let me know your thoughts or if any further changes are needed!

@vyas-git vyas-git linked a pull request Oct 22, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug help wanted pkg-suite Change related to package testify/suite
Projects
None yet
Milestone
No milestone
3 participants
@dolmen @wvell @vyas-git