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

Update ShouldNotEqual and ShouldNotBeNil to not rely on its counterpart #58

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

Conversation

ale7714
Copy link

@ale7714 ale7714 commented Oct 8, 2024

No description provided.

ShouldEqual relies on rendering the actual
and expected values which is not thread safe
causing ShouldNotEqual to be flaky

This change moves checking EqualitySpecs into a
helper function and using that instead.

Signed-off-by: Ale Paredes <[email protected]>
@mdwhatcott
Copy link
Contributor

@ale7714 - thanks for submitting this pull request. Will you go into more detail about what problem are you trying to solve?

@ale7714
Copy link
Author

ale7714 commented Oct 9, 2024

@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!

@mdwhatcott
Copy link
Contributor

@ale7714 - Ah, interesting. I anxiously await any results you can share from your investigation.

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.

2 participants