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

AssertNotCalled() panics because mutex is nil #1208

Closed
gkdr opened this issue Jun 21, 2022 · 8 comments
Closed

AssertNotCalled() panics because mutex is nil #1208

gkdr opened this issue Jun 21, 2022 · 8 comments

Comments

@gkdr
Copy link

gkdr commented Jun 21, 2022

hello,

thank you for your continued work on this library.

i have an issue that for some reason only came up today, but i believe this is a similar issue to #1206. i have one test which only checks that a function was not called using AssertNotCalled(), which now fails because the mutex is nil and testify panics.

i instantiate the mock using new, is that wrong?

i am willing to prepare a PR for you, but i am not sure what the fix is. in #1207, a check is added:

	if m.mutex == nil {
		m.mutex = &sync.Mutex{}
	}

does this need to be added to all of AssertNotCalled(), AssertCalled(), AssertNumberOfCalls()? is that all or are there any other places?

@PrafulFP
Copy link

I think a fix is also needed for IsMethodCallable and MethodCalled.

@Kryvchun
Copy link

The same about AssertNumberOfCalls

@psmarcin
Copy link

Using yourmock.Test method before yourmock.AssertNumberOfCalls populates mutex. It's just temporary workaround because this change looks like breaking change for me.

@yurishkuro
Copy link

Having a similar issue - upgrading to 1.7.4 causes some tests to panic.

Just curious, what was the motivation for changing mutex to a pointer in #1182 ? The PR did not provide any reasoning, and considering that the mocks are usually initialized as plain structs, having a pointer is asking for trouble.

sfc-gh-eraigosa pushed a commit to sfc-gh-eraigosa/testify that referenced this issue Jun 23, 2022
This reverts a change that was made in stretchr#1182
The PR makes m.mutex a pointer which now needs to be checked but it's not checked for nil everywhere.

This should also help with these issues:
- stretchr#1208
- stretchr#1210
@peczenyj
Copy link

I think a fix is also needed for IsMethodCallable and MethodCalled.

Indeed, I have a m.Called() without On on my programs -- the called was not expected, should fail, not panic on nil reference

boyan-soubachov pushed a commit that referenced this issue Jun 24, 2022
* fixing panic in calls to assertion with nil m.mutex

This reverts a change that was made in #1182
The PR makes m.mutex a pointer which now needs to be checked but it's not checked for nil everywhere.

This should also help with these issues:
- #1208
- #1210

* Revert throwing out the lock because other concurrent calls can already have it locked

* fix go vet copy lock by using pointer

* fix obj assignment for passing test
@boyan-soubachov
Copy link
Collaborator

Fixed in latest version (1.7.5), please confirm and let me know if all is good :)

@PrafulFP
Copy link

Working fine now. Thanks 👍🏻

@gkdr
Copy link
Author

gkdr commented Jun 24, 2022

reverting the change to a pointer fixed the issue. thank you!

@gkdr gkdr closed this as completed Jun 24, 2022
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

No branches or pull requests

7 participants