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

Release 1.7.2 causes go vet errors when embedding suite.Suite #1199

Closed
AndreasKl opened this issue Jun 7, 2022 · 5 comments
Closed

Release 1.7.2 causes go vet errors when embedding suite.Suite #1199

AndreasKl opened this issue Jun 7, 2022 · 5 comments

Comments

@AndreasKl
Copy link

AndreasKl commented Jun 7, 2022 •
edited

Our dependapot builds started to fail for some services. We are embedding suite.Suite to create custom integration test suites.

type IntegrationTester struct {
	suite.Suite
}

With the 1.7.2 release and due to adding a sync.Mutex go vet now fails on this code base.

go vet ./...
# github.com/snabble/...
./integration_test.go:17:9: Test_Tokenize passes lock by value: github.com/snabble/...IntegrationTester contains github.com/stretchr/testify/suite.Suite contains sync.RWMutex
@AndreasKl AndreasKl changed the title Release 1.7.2 causes go vet errors when embedding Release 1.7.2 causes go vet errors when embedding suite.Suite Jun 7, 2022
@daniel-meyer-dme daniel-meyer-dme mentioned this issue Jun 8, 2022
Closed
@daniel-meyer-dme
Copy link

We faced the same issue upgrading to 1.7.2. Tried some approaches to fix this but found no way which wouldn't change the way Suite is used so far.

@AgentSmithMatrix AgentSmithMatrix mentioned this issue Jul 22, 2022
Closed
@drichelson
Copy link

I'm seeing this issue on 1.8.0. It looks like the the non-pointer lock is still on master branch

May we re-open this?

@brackendawson
Copy link
Collaborator

The change was reverted because it caused a lot of panics (issues linked in PR). I don't expect this mutex to evert be made into a pointer again. Re-opening this issue would be valid but the fix needs to be different. I think this would be fixed by #1109 which is an API change in suite that is needed to fix multiple concurrency issues, it would have to go in a testify v2.

@drichelson
Copy link

drichelson commented Aug 12, 2022 •
edited

Thanks for the explanation @brackendawson ! I don't have permissions to re-open the issue, so will you please do it so it doesn't get lost? or Maybe @AndreasKl can do it

n-oden added a commit to odenio/avro that referenced this issue Aug 15, 2022
@n-oden
Revert testify dependency to v1.7.1
8274a69 
Testify >= 1.7.2 contains an issue that breaks go vetting when
using custom test suites: stretchr/testify#1199

This is unfortunately not going to be changed in the v1 branch of
testify.  Since the go-avro module's test suite does not seem to
depend on behaviors of later versions of testify, I would like to
propose reverting to v1.7.1
n-oden added a commit to odenio/avro that referenced this issue Aug 15, 2022
@n-oden
Revert testify dependency to v1.7.1
be95516 
Testify >= 1.7.2 contains an issue that breaks go vetting when
using custom test suites: stretchr/testify#1199

This is unfortunately not going to be changed in the v1 branch of
testify.  Since the go-avro module's test suite does not seem to
depend on behaviors of later versions of testify, I would like to
propose reverting to v1.7.1

Signed-off-by: Nathan J. Mehl <n@oden.io>
@n-oden n-oden mentioned this issue Aug 15, 2022
Merged
nrwiersma pushed a commit to hamba/avro that referenced this issue Aug 15, 2022
@n-oden
fix: revert testify dependency to v1.7.1 (#182)
aa0ecca 
Testify >= 1.7.2 contains an issue that breaks go vetting when
using custom test suites: stretchr/testify#1199

This is unfortunately not going to be changed in the v1 branch of
testify.  Since the go-avro module's test suite does not seem to
depend on behaviors of later versions of testify, I would like to
propose reverting to v1.7.1

Signed-off-by: Nathan J. Mehl <n@oden.io>
@drichelson
Copy link

For anyone using golangci-lint this item in the exclude section bypasses these failures:

    - 'github.com\/stretchr\/testify\/suite\.Suite contains sync\.RWMutex'

@shaspitz shaspitz mentioned this issue Nov 17, 2022
Merged
@laurenkrugen-navapbc laurenkrugen-navapbc mentioned this issue Dec 7, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

don't copy mutex
4 participants
@drichelson @AndreasKl @brackendawson @daniel-meyer-dme