Support locking in integration tests #776

mgyucht · 2024-01-23T08:28:54Z

Changes

Locking Interface

Integration tests run in parallel, both within a single action runner as well as across different runs of an action (e.g. multiple people running integration tests in parallel). Some tests modify certain resources and need exclusive access to those resources during the duration of the test. As a result, when two tests that modify a shared resource run concurrently, they fail.

This PR introduces an integration test locking framework for the Go SDK & tools built on the Go SDK. In general, for Databricks integration tests, usage will look like the example in git_credentials_test.go:

func acquireGitCredentialsLock(ctx context.Context, t *testing.T, w *databricks.WorkspaceClient) {
	me, err := w.CurrentUser.Me(ctx)
	require.NoError(t, err)
	lockable := lock.GitCredentials{
		WorkspaceHost: w.Config.Host,
		Username:      me.UserName,
	}
	_, err = lock.Acquire(ctx, lockable, lock.InTest(t))
	require.NoError(t, err)
}

func TestAccGitCredentials(t *testing.T) {
	ctx, w := workspaceTest(t)

	// skip-next-line-roll
	acquireGitCredentialsLock(ctx, t, w)
	...
}

Calling lock.Acquire() will attempt to lock a resource using a lock backend, returning a lock. When used in tests, unlocking the lock will be registered automatically as a test cleanup, so you only need to check that no error occurred while acquiring the lock. By default, the lock will be held for 60 seconds, and locks will be created in a workspace for which the serialized SDK config can be found in LOCK_CLIENT_CONFIG and the path to the lock directory in LOCK_DIRECTORY.

A lockable resource is any resource implementing the Lockable interface, which provides the lock ID to use. I've added several implementations of Lockable for cases that I know about, but any string or structure containing only string/int fields can be used as a Lockable through lock.NewLockableImpl(). If a string is provided, the lock ID is the string itself. If a struct is provided, the lock ID is the struct name followed by the names of each field & the value of each field, separated by semicolon. For example: testStruct:A=hello;B=1.

OpenAPI Code Generator

Additionally, I've added a directive // skip-next-line-roll in the integration test parser. This makes it possible to skip the next statement from the codegen parsing. This should make it easier to add other code to integration tests without needing to update the OpenAPI test compiler. Just add the comment right before the statement you want to skip from example generation, e.g.

func TestMwsAccAbc(t *testing.T) {
	...
	// skip-next-line-roll
	myCustomFunc()
	....
}

Finally, I've made a small bugfix in the comment parsing, which currently sets hint to the last comment before the current node, even if the current node is not immediately preceded by a comment.

Tests

make test passing
make fmt applied
relevant integration tests applied

hectorcast-db · 2024-01-23T10:19:01Z

qa/lock/azure_backend.go

+		return err
+	}
+	b.blobClient = blobClient
+	fmt.Printf("Created a container client object with URL %s\n", blobClient.URL())


Will this print the token?

hectorcast-db · 2024-01-23T10:22:11Z

qa/lock/azure_backend.go

+	// Now acquire a lease on the container.
+	// You can choose to pass an empty string for proposed ID so that the service automatically assigns one for you.
+	leaseDuration := int32(math.Min(60, duration.Seconds()))
+	acquireLeaseResponse, err := blobLeaseClient.AcquireLease(context.TODO(), leaseDuration, nil)


Is TODO on purpose? Maybe some note on what kind of context we want here.

hectorcast-db · 2024-01-23T10:24:32Z

qa/lock/azure_backend.go

+}
+
+func (b *azureBackend) RenewLock(ctx context.Context, leaseId string) error {
+	_, err := b.leaseClient.RenewLease(ctx, nil)


Don't we use the leaseId?

Oh, it's part of the leaseClient in this particular implementation. Maybe add a comment.

hectorcast-db · 2024-01-23T10:26:16Z

qa/lock/azure_backend.go

+	fmt.Printf("Created a blob lease client object with lease ID %s\n", leaseId)
+
+	// Now acquire a lease on the container.
+	// You can choose to pass an empty string for proposed ID so that the service automatically assigns one for you.


Maybe add the comment to the AcquireLock function, since the leaseID is passed there as a parameter

pietern

(incomplete review)

pietern · 2024-01-23T15:27:04Z

qa/lock/core/state.go

+	GithubRunId     string `env:"GITHUB_RUN_ID"`
+	GithubRunNumber string `env:"GITHUB_RUN_NUMBER"`
+	GithubSha       string `env:"GITHUB_SHA"`
+	GithubWorkflow  string `env:"GITHUB_WORKFLOW"`


Is the repository encoded in one of these?

Nope, will add!

pietern · 2024-01-23T15:27:27Z

qa/lock/core/state.go

+}
+
+func (l *LockState) Extend(duration time.Duration) {
+	l.Expiry = time.Now().Add(duration)


Shouldn't this also persist the extended expiry time?

Nevermind, this is done elsewhere.

Could the lock state itself be unexported?

Discussed offline, I'll add doccomments here.

pietern · 2024-01-23T15:28:40Z

qa/lock/databricks/databricks_backend.go

+	}
+
+	opts := []workspace.UploadOption{
+		workspace.UploadLanguage(workspace.LanguagePython),


Can we omit the language setting here?

Nope, otherwise the API thinks this is a ZIP file (probably DBC archive).

pietern · 2024-01-23T15:32:11Z

qa/lock/lockables_test.go

+	"github.com/databricks/databricks-sdk-go/qa/lock/internal"
+)
+
+func TestGenerateBlobName(t *testing.T) {


This test looks like it should live in internal as well (only uses GenerateBlobName).

pietern · 2024-01-23T15:33:20Z

qa/lock/lock.go

+// By default, the lock will be acquired for 1 minute. This can be changed by
+// passing the WithLeaseDuration() LockOption.
+//
+// By default, the lock will be acquired using the azureBackend. This can be


Default is different.

pietern · 2024-01-23T15:34:30Z

qa/lock/lock.go

+		testTimer := time.NewTimer(opts.LeaseDuration)
+		defer close(errs)
+		for {
+			timer.Reset(duration)


Can this be a time.Ticker?

Most certainly.

openapi/roll/tool.go

qa/lock/core/lockable.go

qa/lock/databricks/databricks_backend_test.go

qa/lock/lock.go

qa/lock/lockables_test.go

codecov-commenter · 2024-01-23T16:12:41Z

Codecov Report

Attention: 67 lines in your changes are missing coverage. Please review.

Comparison is base (aad9abf) 18.27% compared to head (f5bc4a6) 18.69%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
qa/lock/databricks/databricks_backend.go	57.14%	25 Missing and 8 partials ⚠️
qa/lock/lock.go	62.96%	17 Missing and 3 partials ⚠️
openapi/roll/tool.go	68.00%	7 Missing and 1 partial ⚠️
qa/lock/internal/lockable.go	92.59%	1 Missing and 1 partial ⚠️
qa/lock/lock_options.go	66.66%	2 Missing ⚠️
qa/lock/lockables.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #776      +/-   ##
==========================================
+ Coverage   18.27%   18.69%   +0.41%     
==========================================
  Files         110      115       +5     
  Lines       14834    15135     +301     
==========================================
+ Hits         2711     2829     +118     
- Misses      11901    12071     +170     
- Partials      222      235      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mgyucht added 5 commits January 23, 2024 00:18

Add locking support

e2ac5a0

allow skipping lines in test autogeneration

c738e53

fix autogeneration

1ef416a

rename func

ac63a3b

make lockable mandatory

ce83991

mgyucht requested review from pietern, tanmay-db and hectorcast-db January 23, 2024 08:29

fix test

55e5b7b

hectorcast-db reviewed Jan 23, 2024

View reviewed changes

mgyucht added 4 commits January 23, 2024 14:32

locks

5b5fed8

fix

25c8604

some bugfixes

9910698

fix tests

39854f0

pietern reviewed Jan 23, 2024

View reviewed changes

mgyucht commented Jan 23, 2024

View reviewed changes

openapi/roll/tool.go Show resolved Hide resolved

qa/lock/core/lockable.go Show resolved Hide resolved

qa/lock/databricks/databricks_backend_test.go Show resolved Hide resolved

qa/lock/lock.go Outdated Show resolved Hide resolved

qa/lock/lockables_test.go Outdated Show resolved Hide resolved

hectorcast-db approved these changes Jan 23, 2024

View reviewed changes

mgyucht added 2 commits January 23, 2024 16:56

most things

460f734

comments

f5bc4a6

return error instead of panicing

9813818

mgyucht added this pull request to the merge queue Jan 24, 2024

Merged via the queue into main with commit a28e2bb Jan 24, 2024
4 checks passed

mgyucht deleted the add-integration-test-locking branch January 24, 2024 07:09

hectorcast-db mentioned this pull request Jan 24, 2024

Release v0.30.0 #780

Merged

tanmay-db mentioned this pull request Jan 31, 2024

Disable acquiring lock while running git integration tests due to invalid parsing #793

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support locking in integration tests #776

Support locking in integration tests #776

mgyucht commented Jan 23, 2024 •

edited

Loading

hectorcast-db Jan 23, 2024

hectorcast-db Jan 23, 2024

hectorcast-db Jan 23, 2024

hectorcast-db Jan 23, 2024

hectorcast-db Jan 23, 2024

pietern left a comment

pietern Jan 23, 2024

mgyucht Jan 23, 2024

pietern Jan 23, 2024

pietern Jan 23, 2024

mgyucht Jan 23, 2024

pietern Jan 23, 2024

mgyucht Jan 23, 2024

pietern Jan 23, 2024

pietern Jan 23, 2024

pietern Jan 23, 2024

mgyucht Jan 23, 2024

codecov-commenter commented Jan 23, 2024

Support locking in integration tests #776

Support locking in integration tests #776

Conversation

mgyucht commented Jan 23, 2024 • edited Loading

Changes

Locking Interface

OpenAPI Code Generator

Tests

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

Choose a reason for hiding this comment

pietern left a comment

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

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

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

codecov-commenter commented Jan 23, 2024

Codecov Report

mgyucht commented Jan 23, 2024 •

edited

Loading