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

Consistenly rebind loop variable in tests #4419

Closed
3 tasks
DimitrisJim opened this issue Aug 22, 2023 · 1 comment · Fixed by #4658
Closed
3 tasks

Consistenly rebind loop variable in tests #4419

DimitrisJim opened this issue Aug 22, 2023 · 1 comment · Fixed by #4658
Labels
good first issue Good for newcomers type: code hygiene Clean up code but without changing functionality or interfaces

Comments

@DimitrisJim
Copy link
Contributor

Summary

Ref: #4413 (comment)

Though in many cases not necessary, this is a nice defensive rebinding to do in order to guard against the possibility of running things in parallel. A quick grep showed me that there's other instances where this is omitted, would be nice to get them consistent.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@DimitrisJim DimitrisJim added good first issue Good for newcomers type: code hygiene Clean up code but without changing functionality or interfaces labels Aug 22, 2023
@srdtrk
Copy link
Member

srdtrk commented Aug 24, 2023

I think in this case, we should also wrap the actual test logic inside a suite.Run like this:

	for _, tc := range testCases {
		tc := tc
		suite.Run(tc.name, func() {
			suite.SetupTest()

			// ...
		})
	}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers type: code hygiene Clean up code but without changing functionality or interfaces
Projects
None yet
2 participants