From 26e851ed01bdf62a69ef3d02dff64874b112fa25 Mon Sep 17 00:00:00 2001 From: Abhishek Pandey Date: Wed, 10 Jan 2024 16:25:44 -0800 Subject: [PATCH] Reduce graph adapter & http wrapper test runtime (#5002) Another retry related optimization. This PR reduces `TestGraphIntgSuite/TestAdapterWrap_retriesConnectionClose` runtime from 150sec to 0.7sec. --- #### Does this PR need a docs update or release note? - [ ] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [x] :no_entry: No #### Type of change - [ ] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [x] :broom: Tech Debt/Cleanup #### Issue(s) * # #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- .../services/m365/api/graph/http_wrapper.go | 21 ++++++++++--- .../m365/api/graph/http_wrapper_test.go | 8 +++++ .../services/m365/api/graph/service_test.go | 30 +++++++++---------- 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/src/pkg/services/m365/api/graph/http_wrapper.go b/src/pkg/services/m365/api/graph/http_wrapper.go index 737d2b61c..57464365d 100644 --- a/src/pkg/services/m365/api/graph/http_wrapper.go +++ b/src/pkg/services/m365/api/graph/http_wrapper.go @@ -17,6 +17,14 @@ import ( "github.com/alcionai/corso/src/pkg/logger" ) +// --------------------------------------------------------------------------- +// HTTP wrapper config +// --------------------------------------------------------------------------- + +const ( + httpWrapperRetryDelay = 3 * time.Second +) + // --------------------------------------------------------------------------- // constructors // --------------------------------------------------------------------------- @@ -60,7 +68,11 @@ func NewHTTPWrapper( cc.apply(hc) - return &httpWrapper{hc, cc} + return &httpWrapper{ + client: hc, + config: cc, + retryDelay: httpWrapperRetryDelay, + } } // NewNoTimeoutHTTPWrapper constructs a http wrapper with no context timeout. @@ -114,7 +126,7 @@ func (hw httpWrapper) Request( // a common expectation. for i := 0; i < hw.config.maxConnectionRetries+1; i++ { if i > 0 { - time.Sleep(3 * time.Second) + time.Sleep(hw.retryDelay) } ctx = clues.Add(ctx, "request_retry_iter", i) @@ -156,8 +168,9 @@ func (hw httpWrapper) Request( type ( httpWrapper struct { - client *http.Client - config *clientConfig + client *http.Client + config *clientConfig + retryDelay time.Duration } customTransport struct { diff --git a/src/pkg/services/m365/api/graph/http_wrapper_test.go b/src/pkg/services/m365/api/graph/http_wrapper_test.go index 5e274c35c..555af8ffd 100644 --- a/src/pkg/services/m365/api/graph/http_wrapper_test.go +++ b/src/pkg/services/m365/api/graph/http_wrapper_test.go @@ -49,6 +49,9 @@ func (suite *HTTPWrapperIntgSuite) TestNewHTTPWrapper() { require.NotNil(t, resp) require.Equal(t, http.StatusOK, resp.StatusCode) + + // Test http wrapper config + assert.Equal(t, httpWrapperRetryDelay, hw.retryDelay) } type mwForceResp struct { @@ -180,6 +183,11 @@ func (suite *HTTPWrapperUnitSuite) TestNewHTTPWrapper_http2StreamErrorRetries() appendMiddleware(&mwResp), MaxConnectionRetries(test.retries)) + // Configure retry delay to reduce test time. Retry delay doesn't + // really matter here since all requests will be intercepted by + // the test middleware. + hw.retryDelay = 0 + _, err := hw.Request(ctx, http.MethodGet, url, nil, nil) require.ErrorAs(t, err, &http2.StreamError{}, clues.ToCore(err)) require.Equal(t, test.expectRetries, tries, "count of retries") diff --git a/src/pkg/services/m365/api/graph/service_test.go b/src/pkg/services/m365/api/graph/service_test.go index ba7bd56b0..c15ec1be3 100644 --- a/src/pkg/services/m365/api/graph/service_test.go +++ b/src/pkg/services/m365/api/graph/service_test.go @@ -226,7 +226,6 @@ func (suite *GraphIntgSuite) TestAdapterWrap_retriesConnectionClose() { ctx, flush := tester.NewContext(t) defer flush() - url := "https://graph.microsoft.com/fnords/beaux/regard" retryInc := 0 // the panics should get caught and returned as errors @@ -243,24 +242,21 @@ func (suite *GraphIntgSuite) TestAdapterWrap_retriesConnectionClose() { suite.credentials.AzureClientID, suite.credentials.AzureClientSecret, count.New(), - appendMiddleware(&alwaysECONNRESET)) + appendMiddleware(&alwaysECONNRESET), + // Configure retry middlewares so that they don't retry on connection reset. + // Those middlewares have their own tests to verify retries. + MaxRetries(-1)) require.NoError(t, err, clues.ToCore(err)) - // Set retry delay to a low value to reduce test runtime. + // Retry delay doesn't really matter here since all requests will be intercepted + // by the test middleware. Set it to 0 to reduce test runtime. aw := adpt.(*adapterWrap) - aw.retryDelay = 10 * time.Millisecond - - // the query doesn't matter - _, err = users.NewItemCalendarsItemEventsDeltaRequestBuilder(url, adpt).Get(ctx, nil) - require.ErrorIs(t, err, syscall.ECONNRESET, clues.ToCore(err)) - require.Equal(t, 16, retryInc, "number of retries") - - retryInc = 0 + aw.retryDelay = 0 // the query doesn't matter _, err = NewService(adpt).Client().Users().Get(ctx, nil) require.ErrorIs(t, err, syscall.ECONNRESET, clues.ToCore(err)) - require.Equal(t, 16, retryInc, "number of retries") + require.Equal(t, 4, retryInc, "number of retries") } func (suite *GraphIntgSuite) TestAdapterWrap_retriesBadJWTToken() { @@ -307,9 +303,10 @@ func (suite *GraphIntgSuite) TestAdapterWrap_retriesBadJWTToken() { appendMiddleware(&alwaysBadJWT)) require.NoError(t, err, clues.ToCore(err)) - // Set retry delay to a low value to reduce test runtime. + // Retry delay doesn't really matter here since all requests will be intercepted + // by the test middleware. Set it to 0 to reduce test runtime. aw := adpt.(*adapterWrap) - aw.retryDelay = 10 * time.Millisecond + aw.retryDelay = 0 // When run locally this may fail. Not sure why it works in github but not locally. // Pester keepers if it bothers you. @@ -384,9 +381,10 @@ func (suite *GraphIntgSuite) TestAdapterWrap_retriesInvalidRequest() { appendMiddleware(&returnsGraphResp)) require.NoError(t, err, clues.ToCore(err)) - // Set retry delay to a low value to reduce test runtime. + // Retry delay doesn't really matter here since all requests will be intercepted + // by the test middleware. Set it to 0 to reduce test runtime. aw := adpt.(*adapterWrap) - aw.retryDelay = 10 * time.Millisecond + aw.retryDelay = 0 table := []struct { name string