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

[vtadmin] test refactors #7641

Merged
merged 11 commits into from Mar 9, 2021

Conversation

ajm188
Copy link
Contributor

@ajm188 ajm188 commented Mar 8, 2021

Description

This PR contains a bunch of miscellaneous refactors to vtadmin tests, namely:

  • use of subtests (t.Run(...)) in places where it was previously missing
  • use of t.Parallel() in most places
  • fixing a test in package discovery that would panic if you tried running it twice, allowing us to run tests now with -count=N for N > 1.
  • bulk replacement of inlined context.Background() calls with a single, shared context per test function

Related Issue(s)

Checklist

  • Should this PR be backported? no
  • Tests were added or are not required
  • Documentation was added or is not required n/a

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

ajm188 added 10 commits March 8, 2021 13:56
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Also, switch `TestRegister` to use nanos to dedupe test cases that are
running highly concurrently.

Signed-off-by: Andrew Mason <amason@slack-corp.com>
… test function

Signed-off-by: Andrew Mason <amason@slack-corp.com>
Copy link
Contributor

@setassociative setassociative left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally this seems positive; the failure on the race check suggests to me there is work to be done around how we handle test bootstrapping and standing up grpc endpoints

@@ -36,18 +36,24 @@ import (
)

func assertImmediateCaller(t *testing.T, im *querypb.VTGateCallerID, expected string) {
t.Helper()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL

@ajm188 ajm188 force-pushed the am_vtadmin_test_refactors branch from 87f2cba to 6f70970 Compare March 9, 2021 01:49
Two changes:
- I missed a couple places to transition from the direct,
  `grpcvtctldserver.NewVtctldServer` to the indirect
  `testutil.NewVtctldServerWithTabletManagerClient`, which guards against
  data races in the tabletmanager protocol registry.
- Added a call to `grpccommon.EnableTracingOpt()` in the `init` of
  vtadmin's tests, to force the one-time write to occur before any
  concurrent tests start up and try to read that value.

Signed-off-by: Andrew Mason <amason@slack-corp.com>
@ajm188 ajm188 force-pushed the am_vtadmin_test_refactors branch from 6f70970 to 3f7e006 Compare March 9, 2021 01:50
@rohit-nayak-ps rohit-nayak-ps merged commit a3fa48c into vitessio:master Mar 9, 2021
@ajm188 ajm188 deleted the am_vtadmin_test_refactors branch March 9, 2021 13:23
@askdba askdba added the Component: VTAdmin VTadmin interface label Mar 10, 2021
@askdba askdba added this to the v10.0 milestone Mar 10, 2021
@doeg doeg added this to In progress in VTAdmin via automation Mar 16, 2021
@doeg doeg moved this from In progress to Done in VTAdmin Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: VTAdmin VTadmin interface
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants