test(azure): concurrency timing test for parallel dispatcher (closes #262) (#873)

* test(azure): DI seam + concurrency timing tests for parallel dispatcher (closes #262)

Introduce four package-level newXxxClientFn vars (defaulting to the real
compute/database/cache/cosmosdb constructors) so tests can inject fake
clients without changing production behaviour.

Add three tests in recommendations_test.go:
- TestGetRecommendations_Parallelism: 4 fakes sleep 100ms each; asserts
  wall-clock < 200ms, proving concurrent dispatch (serial would be ~400ms).
- TestGetRecommendations_OrderPreservation: staggered sleeps force
  out-of-order completion; asserts merged slice is compute->db->cache->cosmos.
- TestGetRecommendations_ErrorIsolation: one fake errors; asserts the other
  three services still contribute recs (no sibling cancellation).

* fix(test/azure): inject savingsplans+advisor to make timing test hermetic

TestGetRecommendations_Parallelism was failing (~300-700ms wall clock vs
200ms threshold) because the savingsplans and advisor goroutines used real
constructors that made ARM network calls, adding unbounded latency outside
the four fakeServiceClient mocks.

Add newSavingsPlansClientFn package-level var (parallel to the other four)
and a getAdvisorRecsFn field on RecommendationsClientAdapter (defaulting to
r.getAdvisorRecommendations, with a nil guard for struct-literal adapters in
existing tests). Wire newSavingsPlansClientFn in GetRecommendations to
replace the direct savingsplans.NewClient call.

In the three concurrency tests (Parallelism, OrderPreservation,
ErrorIsolation) inject noopAdvisorFn and a zero-sleep fakeServiceClient for
savingsplans so all latency comes exclusively from the four injectable mocks.
Tests are now fully hermetic: no real ARM calls, deterministic rec count,
pass consistently under -race.

699 azure provider tests pass; 0 lint issues in touched files.

Author: cristim

providers/azure/recommendations.go

@@ -22,6 +22,43 @@ import (
 	"github.com/LeanerCloud/CUDly/providers/azure/services/savingsplans"
 )
 
+// serviceRecsGetter is the narrow interface satisfied by each per-service
+// client (compute, database, cache, cosmosdb). The interface exists solely to
+// allow tests to substitute fake implementations; production code uses the
+// concrete types via the newXxxClientFn variables below.
+type serviceRecsGetter interface {
+	GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error)
+}
+
+// newComputeClientFn, newDatabaseClientFn, newCacheClientFn,
+// newCosmosDBClientFn, and newSavingsPlansClientFn default to the real
+// constructors and are overridden in tests to inject fakes. The variables are
+// package-level (not fields on the adapter) so that the constructor signature
+// stays unchanged and the injection is limited to the test package that owns
+// the test binary's address space.
+//
+// getAdvisorRecsFn wraps r.getAdvisorRecommendations; it is a field rather
+// than a package-level var so that injection is scoped to the adapter
+// instance and avoids shared-state issues when tests run the advisor path
+// concurrently. Tests that do not need real ARM calls set it to noopAdvisorFn.
+var (
+	newComputeClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter {
+		return compute.NewClient(cred, sub, region)
+	}
+	newDatabaseClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter {
+		return database.NewClient(cred, sub, region)
+	}
+	newCacheClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter {
+		return cache.NewClient(cred, sub, region)
+	}
+	newCosmosDBClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter {
+		return cosmosdb.NewClient(cred, sub, region)
+	}
+	newSavingsPlansClientFn func(azcore.TokenCredential, string, string) serviceRecsGetter = func(cred azcore.TokenCredential, sub, region string) serviceRecsGetter {
+		return savingsplans.NewClient(cred, sub, region)
+	}
+)
+
 // RecommendationsClientAdapter aggregates Azure reservation recommendations across all services.
 //
 // Invariant: subscriptionID must be non-empty. Downstream converters use it as
@@ -31,9 +68,13 @@ import (
 // path is NewRecommendationsClientAdapter; direct struct literals bypass the
 // invariant check and should be confined to tests that deliberately exercise
 // the unvalidated shape.
+//
+// getAdvisorRecsFn defaults to r.getAdvisorRecommendations and may be
+// overridden per-instance in tests to avoid real ARM network calls.
 type RecommendationsClientAdapter struct {
-	cred           azcore.TokenCredential
-	subscriptionID string
+	cred             azcore.TokenCredential
+	subscriptionID   string
+	getAdvisorRecsFn func(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error)
 }
 
 // NewRecommendationsClientAdapter builds a RecommendationsClientAdapter with
@@ -44,10 +85,12 @@ func NewRecommendationsClientAdapter(cred azcore.TokenCredential, subscriptionID
 	if subscriptionID == "" {
 		return nil, fmt.Errorf("azure recommendations: subscriptionID is required")
 	}
-	return &RecommendationsClientAdapter{
+	r := &RecommendationsClientAdapter{
 		cred:           cred,
 		subscriptionID: subscriptionID,
-	}, nil
+	}
+	r.getAdvisorRecsFn = r.getAdvisorRecommendations
+	return r, nil
 }
 
 // GetRecommendations retrieves all Azure reservation recommendations across services.
@@ -109,31 +152,31 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p
 	// Compute (VM) recommendations — subscription-wide.
 	if includeCompute {
 		goService(&computeErr, func() {
-			computeClient := compute.NewClient(r.cred, r.subscriptionID, "")
+			computeClient := newComputeClientFn(r.cred, r.subscriptionID, "")
 			computeRecs, computeErr = computeClient.GetRecommendations(gctx, params)
 		})
 	}
 
 	// Database (SQL) recommendations — subscription-wide.
 	if includeDB {
 		goService(&dbErr, func() {
-			dbClient := database.NewClient(r.cred, r.subscriptionID, "")
+			dbClient := newDatabaseClientFn(r.cred, r.subscriptionID, "")
 			dbRecs, dbErr = dbClient.GetRecommendations(gctx, params)
 		})
 	}
 
 	// Cache (Redis) recommendations — subscription-wide.
 	if includeCache {
 		goService(&cacheErr, func() {
-			cacheClient := cache.NewClient(r.cred, r.subscriptionID, "")
+			cacheClient := newCacheClientFn(r.cred, r.subscriptionID, "")
 			cacheRecs, cacheErr = cacheClient.GetRecommendations(gctx, params)
 		})
 	}
 
 	// CosmosDB (NoSQL) recommendations — subscription-wide.
 	if includeCosmos {
 		goService(&cosmosErr, func() {
-			cosmosClient := cosmosdb.NewClient(r.cred, r.subscriptionID, "")
+			cosmosClient := newCosmosDBClientFn(r.cred, r.subscriptionID, "")
 			cosmosRecs, cosmosErr = cosmosClient.GetRecommendations(gctx, params)
 		})
 	}
@@ -145,16 +188,23 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p
 	// a scheduler change.
 	if includeSP {
 		goService(&spErr, func() {
-			spClient := savingsplans.NewClient(r.cred, r.subscriptionID, "")
+			spClient := newSavingsPlansClientFn(r.cred, r.subscriptionID, "")
 			spRecs, spErr = spClient.GetRecommendations(gctx, params)
 		})
 	}
 
 	// Azure Advisor adds cross-cutting cost recommendations independent of the
 	// per-service Reservation API. Failures here are non-fatal — the per-service
 	// results above are still useful on their own.
+	//
+	// getAdvisorRecsFn defaults to r.getAdvisorRecommendations and may be
+	// replaced per-adapter in tests to avoid real ARM network calls.
+	advisorFn := r.getAdvisorRecsFn
+	if advisorFn == nil {
+		advisorFn = r.getAdvisorRecommendations
+	}
 	goService(&advisorErr, func() {
-		advisorRecs, advisorErr = r.getAdvisorRecommendations(gctx, params)
+		advisorRecs, advisorErr = advisorFn(gctx, params)
 	})
 
 	// Wait for all goroutines. g.Wait() always returns nil because every

providers/azure/recommendations_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
@@ -557,3 +558,192 @@ func TestMergeServiceResults_StubsDoNotMaskTotalFailure(t *testing.T) {
 	assert.Contains(t, err.Error(), "all 4 Azure recommendation services failed")
 	assert.Nil(t, recs)
 }
+
+// fakeServiceClient is a test double for serviceRecsGetter. It sleeps for
+// sleepDur to simulate network latency and then returns the fixed recs slice
+// (or err when non-nil). sleep is done inside GetRecommendations so that
+// mock latency is isolated to the mock body — the test's assert path never
+// sleeps (see memory feedback_no_sleep_in_tests).
+type fakeServiceClient struct {
+	sleepDur time.Duration
+	recs     []common.Recommendation
+	err      error
+}
+
+func (f *fakeServiceClient) GetRecommendations(ctx context.Context, _ common.RecommendationParams) ([]common.Recommendation, error) {
+	select {
+	case <-time.After(f.sleepDur):
+	case <-ctx.Done():
+		return nil, ctx.Err()
+	}
+	return f.recs, f.err
+}
+
+// newFakeFn returns a constructor compatible with the newXxxClientFn signature
+// that ignores the credential/subscription/region and always returns fake.
+func newFakeFn(fake serviceRecsGetter) func(azcore.TokenCredential, string, string) serviceRecsGetter {
+	return func(_ azcore.TokenCredential, _, _ string) serviceRecsGetter { return fake }
+}
+
+// noopAdvisorFn is a getAdvisorRecsFn replacement that returns immediately
+// with zero results, used in timing/isolation tests to keep all latency inside
+// the injectable fakeServiceClient mocks.
+func noopAdvisorFn(_ context.Context, _ common.RecommendationParams) ([]common.Recommendation, error) {
+	return nil, nil
+}
+
+const fakeServiceSleep = 100 * time.Millisecond
+
+// TestGetRecommendations_Parallelism proves that all four service goroutines
+// run concurrently: total wall-clock time must be well under 2x per-service
+// sleep (i.e. less than 200ms) rather than near 4x (400ms sequential).
+//
+// savingsplans and advisor are injected as instant no-ops so that all latency
+// comes from the four fakeServiceClient mocks; without this, both paths make
+// real ARM network calls whose RTT dwarfs the 100ms threshold.
+func TestGetRecommendations_Parallelism(t *testing.T) {
+	origCompute := newComputeClientFn
+	origDatabase := newDatabaseClientFn
+	origCache := newCacheClientFn
+	origCosmos := newCosmosDBClientFn
+	origSP := newSavingsPlansClientFn
+	t.Cleanup(func() {
+		newComputeClientFn = origCompute
+		newDatabaseClientFn = origDatabase
+		newCacheClientFn = origCache
+		newCosmosDBClientFn = origCosmos
+		newSavingsPlansClientFn = origSP
+	})
+
+	rec := func(svc common.ServiceType) common.Recommendation {
+		return common.Recommendation{Provider: common.ProviderAzure, Service: svc}
+	}
+
+	noopFake := newFakeFn(&fakeServiceClient{})
+	newComputeClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceCompute)}})
+	newDatabaseClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceRelationalDB)}})
+	newCacheClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceCache)}})
+	newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{rec(common.ServiceNoSQL)}})
+	newSavingsPlansClientFn = noopFake
+
+	adapter := &RecommendationsClientAdapter{
+		cred:             &mockAzureTokenCredential{},
+		subscriptionID:   "sub-parallelism",
+		getAdvisorRecsFn: noopAdvisorFn,
+	}
+
+	start := time.Now()
+	_, err := adapter.GetRecommendations(context.Background(), common.RecommendationParams{})
+	elapsed := time.Since(start)
+
+	require.NoError(t, err)
+	// With 4 services sleeping 100ms in parallel the wall-clock must be
+	// substantially less than 2x per-service sleep. We allow 190ms headroom
+	// for scheduler jitter; if the calls were serial this would take ~400ms.
+	assert.Less(t, elapsed, 2*fakeServiceSleep,
+		"expected parallel dispatch: elapsed %v >= 2x per-service sleep %v -- services may be running serially",
+		elapsed, fakeServiceSleep)
+}
+
+// TestGetRecommendations_OrderPreservation verifies that the merged slice
+// follows the canonical order compute -> database -> cache -> cosmosdb regardless
+// of which fake goroutine returns first (staggered sleeps force an
+// out-of-start-order completion).
+//
+// savingsplans and advisor are injected as instant no-ops so the test is
+// fully hermetic: no real ARM calls, deterministic rec count.
+func TestGetRecommendations_OrderPreservation(t *testing.T) {
+	origCompute := newComputeClientFn
+	origDatabase := newDatabaseClientFn
+	origCache := newCacheClientFn
+	origCosmos := newCosmosDBClientFn
+	origSP := newSavingsPlansClientFn
+	t.Cleanup(func() {
+		newComputeClientFn = origCompute
+		newDatabaseClientFn = origDatabase
+		newCacheClientFn = origCache
+		newCosmosDBClientFn = origCosmos
+		newSavingsPlansClientFn = origSP
+	})
+
+	makeRec := func(svc common.ServiceType) common.Recommendation {
+		return common.Recommendation{Provider: common.ProviderAzure, Service: svc, ResourceType: string(svc)}
+	}
+
+	// Stagger sleeps so goroutines complete in reverse order: cosmosdb
+	// finishes first (~10ms), compute last (~40ms). The merged slice must
+	// still reflect the canonical order.
+	newComputeClientFn = newFakeFn(&fakeServiceClient{sleepDur: 40 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceCompute)}})
+	newDatabaseClientFn = newFakeFn(&fakeServiceClient{sleepDur: 30 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceRelationalDB)}})
+	newCacheClientFn = newFakeFn(&fakeServiceClient{sleepDur: 20 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceCache)}})
+	newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: 10 * time.Millisecond, recs: []common.Recommendation{makeRec(common.ServiceNoSQL)}})
+	newSavingsPlansClientFn = newFakeFn(&fakeServiceClient{})
+
+	adapter := &RecommendationsClientAdapter{
+		cred:             &mockAzureTokenCredential{},
+		subscriptionID:   "sub-order",
+		getAdvisorRecsFn: noopAdvisorFn,
+	}
+
+	recs, err := adapter.GetRecommendations(context.Background(), common.RecommendationParams{})
+	require.NoError(t, err)
+
+	// savingsplans and advisor are no-ops; we expect exactly 4 recs (one per
+	// injectable service) in canonical order.
+	require.Len(t, recs, 4, "expected one rec per injectable service")
+	assert.Equal(t, common.ServiceCompute, recs[0].Service, "slot 0 must be compute")
+	assert.Equal(t, common.ServiceRelationalDB, recs[1].Service, "slot 1 must be database")
+	assert.Equal(t, common.ServiceCache, recs[2].Service, "slot 2 must be cache")
+	assert.Equal(t, common.ServiceNoSQL, recs[3].Service, "slot 3 must be cosmosdb")
+}
+
+// TestGetRecommendations_ErrorIsolation asserts that a single service error
+// does not prevent the other services' results from appearing in the merged
+// slice (no sibling cancellation).
+//
+// savingsplans and advisor are injected as instant no-ops so the rec count is
+// fully deterministic regardless of network availability.
+func TestGetRecommendations_ErrorIsolation(t *testing.T) {
+	origCompute := newComputeClientFn
+	origDatabase := newDatabaseClientFn
+	origCache := newCacheClientFn
+	origCosmos := newCosmosDBClientFn
+	origSP := newSavingsPlansClientFn
+	t.Cleanup(func() {
+		newComputeClientFn = origCompute
+		newDatabaseClientFn = origDatabase
+		newCacheClientFn = origCache
+		newCosmosDBClientFn = origCosmos
+		newSavingsPlansClientFn = origSP
+	})
+
+	makeRec := func(svc common.ServiceType) common.Recommendation {
+		return common.Recommendation{Provider: common.ProviderAzure, Service: svc}
+	}
+
+	// database returns an error; the other three must still contribute recs.
+	newComputeClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{makeRec(common.ServiceCompute)}})
+	newDatabaseClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, err: errors.New("db unavailable")})
+	newCacheClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{makeRec(common.ServiceCache)}})
+	newCosmosDBClientFn = newFakeFn(&fakeServiceClient{sleepDur: fakeServiceSleep, recs: []common.Recommendation{makeRec(common.ServiceNoSQL)}})
+	newSavingsPlansClientFn = newFakeFn(&fakeServiceClient{})
+
+	adapter := &RecommendationsClientAdapter{
+		cred:             &mockAzureTokenCredential{},
+		subscriptionID:   "sub-isolation",
+		getAdvisorRecsFn: noopAdvisorFn,
+	}
+
+	recs, err := adapter.GetRecommendations(context.Background(), common.RecommendationParams{})
+	require.NoError(t, err, "a per-service error must not surface as a GetRecommendations error")
+	require.Len(t, recs, 3, "expected recs from the 3 healthy injectable services")
+
+	services := make([]common.ServiceType, len(recs))
+	for i, r := range recs {
+		services[i] = r.Service
+	}
+	assert.Contains(t, services, common.ServiceCompute, "compute recs must be present despite db error")
+	assert.Contains(t, services, common.ServiceCache, "cache recs must be present despite db error")
+	assert.Contains(t, services, common.ServiceNoSQL, "cosmosdb recs must be present despite db error")
+	assert.NotContains(t, services, common.ServiceRelationalDB, "db recs must be absent when db errors")
+}