Port `qa.HTTPFixtures` to faster transport-level stubs by nfx · Pull Request #708 · databricks/databricks-sdk-go

nfx · 2023-11-24T14:43:52Z

This PR replicates the similar behavior for HTTP fixtures that required starting new HTTP server for every test, providing the similar usage patterns, but better integrated with the decoupled http client.

Start testing APIs with the following snippet

client := httpclient.NewApiClient(httpclient.ClientConfig{
  Transport: fixtures.SliceTransport{},
})

and follow the error messages to something like this:

	type op struct {
		Foo string `json:"foo"`
	}
	client := httpclient.NewApiClient(httpclient.ClientConfig{
		Transport: fixtures.SliceTransport{
			{
				Method:   "POST",
				Resource: "/foo",
				ExpectedRequest: op{
					Foo: "ba..r",
				},
			},
		},
	})
	ctx := context.Background()
	err := client.Do(ctx, "POST", "/foo", httpclient.WithRequestData(map[string]any{
		"foo": "bar",
	}))
	assert.EqualError(t, err, `Post "/foo": expected request: want {
  "foo": "ba..r"
}, got {
  "foo": "bar"
}`, err.Error())

Usage example for stability improvements:

https://github.com/databricks/databricks-sdk-go/blob/feat/token-refreshers/config/auth_azure_msi_test.go#L20-L40

codecov-commenter · 2023-11-24T14:45:00Z

Codecov Report

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

Comparison is base (b0f3c83) 16.33% compared to head (739c9c2) 16.78%.

Files	Patch %	Lines
httpclient/fixtures/fixture.go	68.88%	7 Missing and 7 partials ⚠️
httpclient/fixtures/stub.go	76.92%	3 Missing and 3 partials ⚠️
httpclient/fixtures/map_transport.go	75.00%	2 Missing and 2 partials ⚠️
httpclient/fixtures/slice_transport.go	80.00%	2 Missing and 2 partials ⚠️
httpclient/api_client.go	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #708      +/-   ##
==========================================
+ Coverage   16.33%   16.78%   +0.45%     
==========================================
  Files          98      102       +4     
  Lines       13995    14107     +112     
==========================================
+ Hits         2286     2368      +82     
- Misses      11518    11533      +15     
- Partials      191      206      +15

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

mgyucht

Couple comments, otherwise looking good to me.

mgyucht · 2023-11-28T12:20:05Z

httpclient/api_client.go

+	var skipRetryOnIO bool
+	// depend on the HTTP fixture interface to prevent any coupling
+	if skippable, ok := c.httpClient.Transport.(interface {
+		SkipRetryOnIO() bool
+	}); ok {
+		skipRetryOnIO = skippable.SkipRetryOnIO()
+	}
 	_, isIO := err.(*url.Error)
-	if isIO {
+	if isIO && !skipRetryOnIO {


Where is this used/needed? I didn't see a reference to it in the linked file.

if we don't do this - we keep retrying for the default 5 minutes.

httpclient/fixtures/map_transport.go

httpclient/fixtures/slice_transport.go

httpclient/fixtures/slice_transport_test.go

Major changes: * There has been a major overhaul of error handling. Users can now compare errors in API responses to the well-known error responses defined in the `apierr` package and reexported in the `databricks` package. Users can check whether a specific error was returned, for example `errors.Is(err, databricks.ErrResourceAlreadyExists)`, rather than converting the error to `*APIError` to check the status code and error code. This change is backwards-compatible; users do not need to modify existing error-handling code when upgrading the SDK. See [#682](#682) and [#703](#703) for the changes and https://github.com/databricks/databricks-sdk-go/blob/main/error_alias.go for the full set of errors. Bug fixes: * Handle "no configuration file found at" error during databricks-cli authentication ([#707](#707)). * Introduce `DatabricksEnvironment` and fix Azure MSI auth from ACR, where IMDS doesn't give host environment information ([#700](#700)). * Fix SCIM Pagination default parameters in the Go SDK ([#717](#717)). Other changes: * Update `slog` example with the correct interface ([#694](#694)). * Fixed typo in error message for unknown azure environment ([#701](#701)). * Allow injection of HTTP transport to enable HTTP replayer pattern ([#697](#697)). * Decouple HTTP retries and error mapping mechanics from `DatabricksClient` into `httpclient.ApiClient` ([#699](#699), [#702](#702), [#712](#712)). * Port `qa.HTTPFixtures` to faster transport-level stubs ([#708](#708)). API Changes: * Removed `EnableOptimization` method for [w.Metastores](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#MetastoresAPI) workspace-level service. * Added `PipelineId` field for [catalog.TableInfo](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#TableInfo). * Added `EnablePredictiveOptimization` field for [catalog.UpdateCatalog](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#UpdateCatalog) and [catalog.UpdateSchema](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#UpdateSchema). * Removed [catalog.UpdatePredictiveOptimization](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#UpdatePredictiveOptimization) and [catalog.UpdatePredictiveOptimizationResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/catalog#UpdatePredictiveOptimizationResponse). * Added `Description` field for [jobs.CreateJob](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/jobs#CreateJob) and [jobs.JobSettings](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/jobs#JobSettings). * Added `ListNetworkConnectivityConfigurations` and `ListPrivateEndpointRules` method for [a.NetworkConnectivity](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#NetworkConnectivityAPI) account-level service. * Added [settings.ListNccAzurePrivateEndpointRulesResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListNccAzurePrivateEndpointRulesResponse), [settings.ListNetworkConnectivityConfigurationsRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListNetworkConnectivityConfigurationsRequest), [settings.ListNetworkConnectivityConfigurationsResponse](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListNetworkConnectivityConfigurationsResponse), and [settings.ListPrivateEndpointRulesRequest](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/settings#ListPrivateEndpointRulesRequest). * Added `StringSharedAs` field for [sharing.SharedDataObject](https://pkg.go.dev/github.com/databricks/databricks-sdk-go/service/sharing#SharedDataObject). Internal changes: * Added `contains` method in OpenAPI Generator ([#690](#690)). * Skip recipients tests in Azure ([#692](#692)). * Allow Files API tests to run in UC environments ([#695](#695)). * More cleanup in Unity Catalog integration test ([#719](#719)). OpenAPI SHA: 22f09783eb8a84d52026f856be3b2068f9498db3, Date: 2023-11-23 Dependency updates: * Bump golang.org/x/oauth2 from 0.13.0 to 0.14.0 ([#689](#689)). * Bump google.golang.org/api from 0.150.0 to 0.151.0 ([#698](#698)). * Bump the OpenAPI Spec ([#706](#706)). * Bump golang.org/x/oauth2 from 0.14.0 to 0.15.0 ([#715](#715)). * Bump golang.org/x/time from 0.4.0 to 0.5.0 ([#714](#714)). * Bump google.golang.org/api from 0.151.0 to 0.152.0 ([#716](#716)).

…d add 100% test coverage (#709) This PR improves the stability for Azure MSI authentication by adopting the httpclient transport. Needs two PRs merged first: - #708 - #700

nfx added the do-not-merge label Nov 24, 2023

nfx requested a review from mgyucht November 24, 2023 14:43

Base automatically changed from httpclient to main November 24, 2023 16:45

Port qa.HTTPFixtures to faster transport-level stubs

ff983e2

nfx force-pushed the http-transport-fixtures branch from 9d7ab61 to ff983e2 Compare November 24, 2023 16:52

nfx mentioned this pull request Nov 24, 2023

Migrate Azure MSI & Metadata Service token sources to httpclient and add 100% test coverage #709

Merged

..

44c0ed8

nfx removed the do-not-merge label Nov 24, 2023

nfx enabled auto-merge November 24, 2023 19:44

added map transport

739c9c2

nfx requested review from hectorcast-db and tanmay-db November 25, 2023 13:54

nfx disabled auto-merge November 27, 2023 09:28

hectorcast-db approved these changes Nov 28, 2023

View reviewed changes

mgyucht reviewed Nov 28, 2023

View reviewed changes

nfx added this pull request to the merge queue Nov 28, 2023

Merged via the queue into main with commit f7a5e46 Nov 28, 2023

nfx deleted the http-transport-fixtures branch November 28, 2023 12:58

mgyucht mentioned this pull request Nov 29, 2023

Release v0.26.0 #720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port `qa.HTTPFixtures` to faster transport-level stubs#708

Port `qa.HTTPFixtures` to faster transport-level stubs#708
nfx merged 3 commits intomainfrom
http-transport-fixtures

nfx commented Nov 24, 2023 •

edited

Loading

Uh oh!

codecov-commenter commented Nov 24, 2023 •

edited

Loading

Uh oh!

mgyucht left a comment

Uh oh!

mgyucht Nov 28, 2023

Uh oh!

nfx Nov 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

nfx commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mgyucht left a comment

Choose a reason for hiding this comment

Uh oh!

mgyucht Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

nfx Nov 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nfx commented Nov 24, 2023 •

edited

Loading

codecov-commenter commented Nov 24, 2023 •

edited

Loading