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

Validate suite method signatures and continue execution for valid tests #1665

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

Conversation

vyas-git
Copy link

Summary

This PR improves method signature validation in Testify's suite execution by ensuring that invalid methods are logged, and valid methods continue to execute.

Changes

  • Added validation of method signatures (checking for arguments and return values) at an earlier stage, after method filtering.
  • Replaced t.Fatalf with t.Errorf to report invalid method signatures while allowing the test suite to continue executing valid methods.
  • Introduced additional test cases to cover both valid and invalid method signatures.

Motivation

  • To ensure that invalid method signatures in the suite are reported without stopping execution, allowing valid methods to run and improving overall test suite behavior.

Related issues

Copy link
Collaborator

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

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

This error shows up when you run a valid test. Previously there is no panic in this scenario.

eg:

package kata_test

import (
	"testing"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/suite"
)

type ExampleTestSuite struct {
	suite.Suite
	VariableThatShouldStartAtFive int
}

func (suite *ExampleTestSuite) SetupTest() {
	suite.VariableThatShouldStartAtFive = 5
}

func (suite *ExampleTestSuite) TestExample() {
	assert.Equal(suite.T(), 6, suite.VariableThatShouldStartAtFive)
}

func (suite *ExampleTestSuite) TestSomething(a any) {
	suite.Equal(1, a)
}

func TestExampleTestSuite(t *testing.T) {
	suite.Run(t, new(ExampleTestSuite))
}
% go test -run=TestExampleTestSuite/TestExample
--- FAIL: TestExampleTestSuite (0.00s)
    suite.go:153: testify: suite method 'TestSomething' has 1 input arguments and 0 return values. It should have none.
FAIL
exit status 1
FAIL    github.com/brackendawson/kata   0.280s

@vyas-git
Copy link
Author

vyas-git commented Nov 1, 2024

@brackendawson Thank you for pointing that out! I understand the concern.

Since t is tied to the suite instance, this caused the entire suite to be marked as failed whenever an invalid method was detected—even when running a specific test like TestExampleTestSuite/TestExample.

The suite considers all its methods, and methodFilter cannot filter down to a specific test.

I considered using t.Skip within a testing.InternalTest case, but creating extra tests for invalid methods just to call t.Skip felt unnecessary. Skipping invalid methods early with a warning message turned out to be a cleaner solution.

To address this, I updated the approach to log a warning message directly and skip invalid methods without using t.Errorf. This change ensures:

  1. Invalid methods are skipped, but valid methods continue to execute without marking the entire suite as failed.
  2. Only the requested test methods are executed, while the suite logs a warning message for any invalid method signatures encountered.

Thank you for your feedback, please let me know if this approach aligns with the intended behavior or if there are further adjustments you’d like to see!

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.

suite: missing signature check before calling Test* method
2 participants