smarty
Update ShouldNotEqual and ShouldNotBeNil to not rely on its counterpart
@ale7714 - thanks for submitting this pull request. Will you go into more detail about what problem are you trying to solve?
@mdwhatcott Oops—I apologize, my intention was to open this PR against my fork to discuss with my team before proposing it upstream. I appreciate your quick response and want to provide more context for the changes. We’ve experienced some flaky test failures in our codebase that seem related to concurrency issues. Here’s an example of a stack trace we've seen:
fmt.Sprintf()
/usr/lib/go-1.21/src/fmt/print.go:239 +0x5c
github.com/smartystreets/assertions.composeEqualityMismatchMessage()
/home/testbot/go/pkg/mod/github.com/smartystreets/[email protected]/equality.go:44 +0x105
github.com/smartystreets/assertions.shouldEqual()
/home/testbot/go/pkg/mod/github.com/smartystreets/[email protected]/equality.go:39 +0x277
github.com/smartystreets/assertions.ShouldEqual()
/home/testbot/go/pkg/mod/github.com/smartystreets/[email protected]/equality.go:24 +0x124
github.com/smartystreets/assertions.ShouldNotEqual()
It seems the issue arises from constructing the failure message in the equality checks, particularly when using fmt.Sprintf() in a concurrent context, as it’s not thread-safe. Specifically, in functions like ShouldNotEqual and ShouldNotBeNil, the values are compared, but constructing the equality mismatch message when there’s a failure can introduce race conditions in a multi-goroutine environment.
My proposed change tries to simplify this logic so once the values are validated, the function can return early when the equality condition is met, without triggering unnecessary string formatting operations. This should reduce the chances of thread-safety issues.
Regardless, I’d like to validate that this change addresses the flaky tests in our codebase, and I will share the results with you once I have more data. I could be missing some nuances or edge cases here, and I’m happy to discuss this further with you before proceeding. Let me know your thoughts on this approach and thanks!
@ale7714 - Ah, interesting. I anxiously await any results you can share from your investigation.