Fix copying suite.Suite in integration tests #5481

3vilhamster · 2023-12-16T00:30:28Z

Fix integration suite copying suite while passing it around

What changed?
Changed TestBase and IntegrationBase to be passed as a pointer to avoid copying suite.Suite internals. That can cause side effects like missing links to test initiated and internal mutex is copied together with the struct.

Why?
Lint warning, though I see a lot of logs in integration tests that likely are leaking due to this copying.

How did you test it?
run integration tests

Potential risks

Release notes

Documentation Changes

taylanisikdemir · 2023-12-16T03:29:19Z

host/testcluster.go

@@ -27,10 +27,8 @@ import (
 	"testing"
 	"time"

-	"github.com/uber/cadence/common/log/loggerimpl"
-
+	"github.com/startreedata/pinot-client-go/pinot"


nit: import groups should ideally go in following order

builtin packages

repo specific packages

external packages

named packages

I'm using default goimports behaviour.
As far as I've seen discussions about grouping, the outcome is that there should be either 2 or 3 groups. But in Uber style guide, we've settled in 2 groups. https://github.com/uber-go/guide/blob/master/style.md#import-group-ordering

broadly I agree, but tbh I don't think it matters much, and it doesn't affect behavior as of [some semi-recent Go version]. automate it or ignore it, imo, it's hidden in almost all editors anyway and it doesn't make code review any more difficult.

It's still a bit surprising to me that goimports doesn't have an option to forcibly reorder stuff... but this looks like it might be reasonable: https://github.com/incu6us/goimports-reviser

I might see if that does what we want. The less we have to touch here, the better.

well. it's like 99% good... but it strips comments on anything it moves :|
if that's fixed it seems quite promising tho. probably not too hard, astutil.CommentMap addresses exactly this kind of thing.

I'd vote to put whatever behaviour we settle on in a linter and just enforce it at the lint level

There is also this https://github.com/NonLogicalDev/gofancyimports which is pretty good. It is mentioned in a issue for goimports golang/go#20818, though it is quite fresh.

Unfortunately that even in monorepo there is no tooling to enforce proper imports.

gofancyimports looks great at a glance:

keeps comments

enforces grouping

drops import ( "single/thing" ) parenthesis

comments above an import keeps that group of imports organized together (manual grouping is possible when desired)

comments on the same line as an import get reordered with other imports

I can get a PR for that up today, it's pretty simple. just gotta verify the changes/license/etc first.

taylanisikdemir · 2023-12-16T03:30:57Z

common/persistence/persistence-tests/persistenceTestBase.go

 	logger, err := loggerimpl.NewDevelopment()
 	if err != nil {
 		panic(err)
 	}

-	return TestBase{
+	return &TestBase{


looks like some other tests depend on this too. unit tests are failing.

Yes, it turns out that most of persistence tests use this and copy suite all the time. Fixed

Groxx

huh. TIL that go vet's at-test-time checks don't include copy-lock checks... that's quite surprising, and yeah there are loads of copy-lock problems in these tests D:

we should deeeeefinitely add go vet -copylocks ./... to our PR-blocking lint checks. too many failures at the moment though.
thankfully they're all in tests except for common/persistence/nosql, and those might be safe in practice (but still absolutely deserve fixing, there's no reason at all to allow that to exist)

Groxx · 2023-12-17T00:41:00Z

common/domain/replicationTaskExecutor_test.go

+	if testing.Verbose() {
+		log.SetOutput(os.Stdout)
+	}


does using t.Log-proxied stuff work, or do these run into races if you do that? I know it's not directly log compatible, but I doubt that matters in practice for our tests.

otherwise:

default stderr -> verbose stdout: essentially matches go test -v (stdout) so 👍 that seems reasonable.

to match -v more closely, do we want to maybe io.Discard when not verbose? I know that's not the same since it will print nothing on non-verbose failure, but it feels closer to the goal of non-verbose for go test ./... purposes. plus none of our existing stuff treats stdout vs stderr differently, and CI is verbose, so I don't know if there's much benefit to specializing it like this unless it's going to lead to a pragmatic or technical improvement somewhere.

this seems broadly useful, given all the bare log use, and this one test doesn't seem special at all at a glance (is it in practice?). seems worth adding to a test-helper package and using everywhere?

In the following PR I want to move all logging into the root suite TestBase/IntegrationBase part and use -v to either use zaptest or use zap.NewDevelopment depending on the -v. At least that is my plan.

if zaptest doesn't cause data races (due to unclean shutdown), always prefer zaptest. there's basically no reason to use zap.NewDevelopment or log.* or fmt.Println in tests, t.Log-based stuff is massively better.

Groxx · 2023-12-17T00:43:09Z

common/persistence/nosql/nosqlplugin/common.go

+	cadenceIndex := strings.LastIndex(cadencePackageDir, "cadence/")
+	cadencePackageDir = cadencePackageDir[:cadenceIndex+len("cadence/")]


was this failing somewhere, or just felt like removing / prefixes? seems fine either way since it doesn't change the behavior (afaict), just odd to see it change, curious if there is some scenario it improves.

generally I think "dir means it has a / suffix already" is reasonable, I just don't know if we have any established patterns here.

My local folder is called my-cadence :D and it failed. I've tried switching to finding git root, but apparently our CI is running on windows and has some issues with safe-folder related stuff. So I just fixed the / part.

our ci is definitely on linux, so it's not that at least. either way this seems totally fine tho.

we could eliminate this entirely if we used //go:embed for the indexes, but that's a separate thing.

Well, from the errors like from
git rev-parse --show-toplevel
that I've got when using https://github.com/Integralist/go-findroot/blob/master/find/find.go#L36
fatal: detected dubious ownership of /cadence
means that we are running on windows with WSL.

Groxx

I suspect that not all of these are necessary on a technical level (ephemeral copies e.g. on return are safe and go vet does not highlight them either)... but using a pointer for all these seems entirely reasonable too, and also more copy-bug-resistant (it's very clear that copies are not being used for intentional behavior here), so 👍

I'm game to approve now and let you merge with whatever since it seems all safe + a clear improvement, and/or feel free to re-request review if ya want more input after changes.

3vilhamster requested review from Shaddoll, neil-xie, davidporter-id-au, Groxx, shijiesheng, agautam478, jakobht, sankari165, dkrotx, taylanisikdemir and demirkayaender as code owners December 16, 2023 00:30

taylanisikdemir approved these changes Dec 16, 2023

View reviewed changes

3vilhamster changed the title ~~Fix integration test base usage~~ Fix copying suite.Suite in integration tests Dec 16, 2023

Groxx reviewed Dec 16, 2023

View reviewed changes

Groxx reviewed Dec 17, 2023

View reviewed changes

Groxx approved these changes Dec 17, 2023

View reviewed changes

3vilhamster force-pushed the fix-test-base branch from a7d7b04 to f1413f9 Compare December 18, 2023 10:00

3vilhamster added 8 commits December 18, 2023 12:13

Fix integration suite copying suite while passing it around

6f1de74

fix copying TestBase all the time

25e9725

avoid copying suite in IntegrationBase

76d600b

fix testBase usage

c908760

fix testsuite usage and find root in tests

8986d45

fix test and debug error of root folder

5db95ad

debug get root

ef9ba8b

switch back from git to getcwd

45244f4

3vilhamster force-pushed the fix-test-base branch from f1413f9 to 45244f4 Compare December 18, 2023 11:13

3vilhamster merged commit e55cb82 into uber:master Dec 18, 2023
16 checks passed

3vilhamster deleted the fix-test-base branch December 18, 2023 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix copying suite.Suite in integration tests #5481

Fix copying suite.Suite in integration tests #5481

3vilhamster commented Dec 16, 2023 •

edited

taylanisikdemir Dec 16, 2023

3vilhamster Dec 16, 2023

Groxx Dec 16, 2023

Groxx Dec 17, 2023

Groxx Dec 17, 2023 •

edited

davidporter-id-au Dec 17, 2023

3vilhamster Dec 18, 2023 •

edited

Groxx Dec 18, 2023 •

edited

taylanisikdemir Dec 16, 2023

3vilhamster Dec 16, 2023

Groxx left a comment

Groxx Dec 17, 2023 •

edited

3vilhamster Dec 18, 2023

Groxx Dec 18, 2023

Groxx Dec 17, 2023 •

edited

3vilhamster Dec 18, 2023

Groxx Dec 18, 2023

3vilhamster Dec 18, 2023

Groxx left a comment •

edited

		cadenceIndex := strings.LastIndex(cadencePackageDir, "cadence/")
		cadencePackageDir = cadencePackageDir[:cadenceIndex+len("cadence/")]

Fix copying suite.Suite in integration tests #5481

Fix copying suite.Suite in integration tests #5481

Conversation

3vilhamster commented Dec 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Groxx Dec 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3vilhamster Dec 18, 2023 • edited

Choose a reason for hiding this comment

Groxx Dec 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Groxx left a comment

Choose a reason for hiding this comment

Groxx Dec 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Groxx Dec 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Groxx left a comment • edited

Choose a reason for hiding this comment

3vilhamster commented Dec 16, 2023 •

edited

Groxx Dec 17, 2023 •

edited

3vilhamster Dec 18, 2023 •

edited

Groxx Dec 18, 2023 •

edited

Groxx Dec 17, 2023 •

edited

Groxx Dec 17, 2023 •

edited

Groxx left a comment •

edited