test(coverage): assert CE lookback window width + add --rec-lookback-period flag (refs #360)

Gap 1: extend mockCoverageCE to capture the last CE TimePeriod and add
TestGetRICoverageMap_LookbackWindowWidth, which parses Start/End YYYY-MM-DD
strings and asserts end-start == lookbackDays. Test fails if the window math
regresses to a hardcoded value.

Gap 2: replace the tautological "custom lookback stored in Config" subcase in
TestFetchExistingCoverage_LookbackDays (which asserted a struct field equals
what was just assigned) with a comment pointing to the new discriminating test.

Gap 3: implement the second flag from issue #360: --rec-lookback-period (7d/30d/60d,
default 7d) controls LookbackPeriodInDays in GetReservationPurchaseRecommendation.
Wired via Config.RecLookbackPeriod through fetchRecommendationsForRegion (CLI path)
and Client.SetRecLookbackPeriod (discovery/UI path). Validated in validateFlags.
TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation asserts all
three enum values reach the actual CE input. TestValidateRecLookbackPeriod guards
the validation logic.

Author: cristim

cmd/main.go

@@ -94,6 +94,11 @@ type Config struct {
 	// 100% coverage). Zero (default) keeps all pools. SPs and recs
 	// without a per-hour signal pass through unfiltered.
 	MinPoolSize float64
+	// RecLookbackPeriod controls the LookbackPeriodInDays passed to
+	// GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d".
+	// A longer window smooths seasonal spikes; a shorter window weights recent
+	// demand more heavily. Default "7d" matches the CE console default.
+	RecLookbackPeriod string
 }
 
 func main() {
@@ -171,6 +176,10 @@ func init() {
 			"below this threshold. Useful with --target-coverage to skip tiny pools "+
 			"that integer arithmetic forces above target (e.g. avg=1 cannot hit 80%%). "+
 			"Default 0 = no filter.")
+	rootCmd.Flags().StringVar(&toolCfg.RecLookbackPeriod, "rec-lookback-period", "7d",
+		"Historical window for GetReservationPurchaseRecommendation. "+
+			"Valid values: 7d, 30d, 60d. A longer window smooths seasonal spikes; "+
+			"a shorter window weights recent demand more heavily. Default 7d.")
 }
 
 // Package-level Config that cobra flags bind to

cmd/multi_service.go

@@ -110,6 +110,9 @@ func runToolMultiService(ctx context.Context, cfg Config) {
 
 	accountCache := NewAccountAliasCache(awsCfg)
 	recClient := awsprovider.NewRecommendationsClient(awsCfg)
+	if adapter, ok := recClient.(*awsprovider.RecommendationsClientAdapter); ok && cfg.RecLookbackPeriod != "" {
+		adapter.SetRecLookbackPeriod(cfg.RecLookbackPeriod)
+	}
 	engineData := fetchEngineVersionData(ctx, cfg)
 
 	// Fetch existing-RI coverage so --target-coverage can subtract what

cmd/multi_service_coverage_test.go

@@ -797,9 +797,11 @@ func TestFetchExistingCoverage_LookbackDays(t *testing.T) {
 		assert.Nil(t, got, "non-AWS provider must return nil (no CE integration)")
 	})
 
-	t.Run("custom lookback stored in Config", func(t *testing.T) {
-		cfg := Config{TargetCoverage: 80, CoverageLookbackDays: 60, Regions: []string{"us-east-1"}}
-		// CoverageLookbackDays field value is preserved in the struct.
-		assert.Equal(t, 60, cfg.CoverageLookbackDays)
-	})
+	// The previous "custom lookback stored in Config" subcase asserted only that
+	// a struct field equals what was just assigned -- a tautology that passes even
+	// if CoverageLookbackDays is never forwarded to GetRICoverageMap.  The real
+	// assertion -- that the lookback value reaches the CE TimePeriod -- is covered
+	// by TestGetRICoverageMap_LookbackWindowWidth in
+	// providers/aws/recommendations/coverage_test.go, which directly verifies
+	// end-start == lookbackDays on the actual CE input.  No redundant subcase here.
 }

cmd/multi_service_helpers.go

@@ -421,12 +421,16 @@ func fetchRecommendationsForRegion(
 		termStr = "3yr"
 	}
 
+	lookback := cfg.RecLookbackPeriod
+	if lookback == "" {
+		lookback = "7d"
+	}
 	params := common.RecommendationParams{
 		Service:        service,
 		Region:         region,
 		PaymentOption:  cfg.PaymentOption,
 		Term:           termStr,
-		LookbackPeriod: "7d",
+		LookbackPeriod: lookback,
 		// Savings Plans specific filters
 		IncludeSPTypes: cfg.IncludeSPTypes,
 		ExcludeSPTypes: cfg.ExcludeSPTypes,

cmd/validators.go

@@ -29,6 +29,19 @@ func validateFlags(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
+	if err := validateRecLookbackPeriod(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// validateRecLookbackPeriod validates the --rec-lookback-period flag.
+func validateRecLookbackPeriod() error {
+	valid := map[string]bool{"7d": true, "30d": true, "60d": true}
+	if !valid[toolCfg.RecLookbackPeriod] {
+		return fmt.Errorf("invalid rec-lookback-period %q: must be one of 7d, 30d, 60d", toolCfg.RecLookbackPeriod)
+	}
 	return nil
 }
 

cmd/validators_test.go

@@ -540,3 +540,39 @@ func TestValidateCoverageLookbackDays(t *testing.T) {
 		})
 	}
 }
+
+// TestValidateRecLookbackPeriod verifies that validateRecLookbackPeriod accepts
+// the three valid values and rejects anything else, including empty string.
+func TestValidateRecLookbackPeriod(t *testing.T) {
+	tests := []struct {
+		name      string
+		period    string
+		wantErr   bool
+		errSubstr string
+	}{
+		{name: "7d valid", period: "7d", wantErr: false},
+		{name: "30d valid", period: "30d", wantErr: false},
+		{name: "60d valid", period: "60d", wantErr: false},
+		{name: "empty rejected", period: "", wantErr: true, errSubstr: "invalid rec-lookback-period"},
+		{name: "14d rejected", period: "14d", wantErr: true, errSubstr: "invalid rec-lookback-period"},
+		{name: "90d rejected", period: "90d", wantErr: true, errSubstr: "invalid rec-lookback-period"},
+		{name: "SEVEN_DAYS rejected", period: "SEVEN_DAYS", wantErr: true, errSubstr: "invalid rec-lookback-period"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			origCfg := toolCfg
+			defer func() { toolCfg = origCfg }()
+			toolCfg.RecLookbackPeriod = tt.period
+			err := validateRecLookbackPeriod()
+			if tt.wantErr {
+				if err == nil {
+					t.Errorf("validateRecLookbackPeriod() expected error containing %q, got nil", tt.errSubstr)
+				} else if tt.errSubstr != "" && !strings.Contains(err.Error(), tt.errSubstr) {
+					t.Errorf("validateRecLookbackPeriod() error = %v, want substring %q", err, tt.errSubstr)
+				}
+			} else if err != nil {
+				t.Errorf("validateRecLookbackPeriod() unexpected error = %v", err)
+			}
+		})
+	}
+}

providers/aws/recommendations/client.go

@@ -49,6 +49,10 @@ type Client struct {
 	// lazily once per Client lifetime via sync.Once (one DescribeInstanceTypes
 	// fan-out per scheduler tick).
 	skuCatalog skuCatalog
+
+	// recLookbackPeriod is forwarded to GetReservationPurchaseRecommendation
+	// as LookbackPeriodInDays. Defaults to "7d" when empty.
+	recLookbackPeriod string
 }
 
 // NewClient creates a new recommendations client
@@ -94,7 +98,7 @@ func (c *Client) SetInstanceTypePagerFactory(f func() InstanceTypePager) {
 // instanceTypeLookup returns the cached SKU entry for instanceType.
 // On the first call the catalogue is built by calling the pager factory.
 // ok=false when no factory is configured, the catalogue fetch failed, or
-// the instance type was not in the catalogue — the caller falls back to
+// the instance type was not in the catalogue -- the caller falls back to
 // VCPU=0/MemoryGB=0 (graceful-degradation contract from Azure PR #810).
 func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (instanceTypeSKUEntry, bool) {
 	if c.instanceTypePagerFactory == nil {
@@ -103,6 +107,13 @@ func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (i
 	return c.skuCatalog.lookup(ctx, instanceType, c.instanceTypePagerFactory)
 }
 
+// SetRecLookbackPeriod configures the LookbackPeriodInDays used by
+// GetRecommendationsForService. Valid values: "7d", "30d", "60d".
+// An empty or unrecognised value falls back to "7d" at call time.
+func (c *Client) SetRecLookbackPeriod(period string) {
+	c.recLookbackPeriod = period
+}
+
 // GetRecommendations fetches Reserved Instance recommendations for any service
 func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) {
 	// Handle Savings Plans separately — they use a different Cost Explorer API
@@ -248,11 +259,15 @@ func (c *Client) GetRecommendationsForService(ctx context.Context, service commo
 				return nil, ctx.Err()
 			}
 			attempts++
+			lookback := c.recLookbackPeriod
+			if lookback == "" {
+				lookback = "7d"
+			}
 			params := common.RecommendationParams{
 				Service:        service,
 				PaymentOption:  payment,
 				Term:           term,
-				LookbackPeriod: "7d",
+				LookbackPeriod: lookback,
 				Region:         "",
 			}
 			recs, err := c.GetRecommendations(ctx, params)

providers/aws/recommendations/client_test.go

@@ -819,3 +819,37 @@ func TestGetRecommendations_RI_PaginationCapError(t *testing.T) {
 	assert.Equal(t, maxRecommendationPages, mock.calls,
 		"must stop exactly at the cap, not one page later")
 }
+
+// TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation asserts
+// that SetRecLookbackPeriod propagates the chosen period into the
+// LookbackPeriodInDays field of every GetReservationPurchaseRecommendation
+// call issued by GetRecommendationsForService (refs #360). This test is
+// intentionally discriminating: it would fail if recLookbackPeriod were
+// ignored and the hardcoded "7d" default were sent instead.
+func TestSetRecLookbackPeriod_ReachesGetReservationPurchaseRecommendation(t *testing.T) {
+	cases := []struct {
+		period   string
+		wantEnum types.LookbackPeriodInDays
+	}{
+		{"7d", types.LookbackPeriodInDaysSevenDays},
+		{"30d", types.LookbackPeriodInDaysThirtyDays},
+		{"60d", types.LookbackPeriodInDaysSixtyDays},
+	}
+	for _, tc := range cases {
+		t.Run(tc.period, func(t *testing.T) {
+			mock := &mockCostExplorerAPI{
+				riRecommendations: &costexplorer.GetReservationPurchaseRecommendationOutput{},
+			}
+			client := NewClientWithAPI(mock, "us-east-1")
+			client.SetRecLookbackPeriod(tc.period)
+
+			_, err := client.GetRecommendationsForService(context.Background(), common.ServiceEC2)
+			require.NoError(t, err)
+			require.NotEmpty(t, mock.riCalls, "GetReservationPurchaseRecommendation must have been called")
+			for i, call := range mock.riCalls {
+				assert.Equal(t, tc.wantEnum, call.LookbackPeriodInDays,
+					"call[%d]: LookbackPeriodInDays must match --rec-lookback-period=%s", i, tc.period)
+			}
+		})
+	}
+}

providers/aws/recommendations/coverage_test.go

@@ -3,6 +3,7 @@ package recommendations
 import (
 	"context"
 	"testing"
+	"time"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/costexplorer"
@@ -15,15 +16,21 @@ import (
 
 // mockCoverageCE extends the test mock with a configurable GetReservationCoverage
 // response so the coverage path can be exercised without hitting AWS.
+// lastTimePeriod captures the TimePeriod from the most recent call so tests can
+// assert that the CE window matches the requested lookbackDays.
 type mockCoverageCE struct {
 	mockCostExplorerAPI
 	coverageOutput *costexplorer.GetReservationCoverageOutput
 	coverageError  error
 	coverageCalls  int
+	lastTimePeriod *types.DateInterval
 }
 
 func (m *mockCoverageCE) GetReservationCoverage(ctx context.Context, params *costexplorer.GetReservationCoverageInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationCoverageOutput, error) {
 	m.coverageCalls++
+	if params != nil {
+		m.lastTimePeriod = params.TimePeriod
+	}
 	if m.coverageError != nil {
 		return nil, m.coverageError
 	}
@@ -118,6 +125,55 @@ func TestGetRICoverageMap_LookbackDefault(t *testing.T) {
 	assert.Equal(t, wantCalls, mock.coverageCalls)
 }
 
+// TestGetRICoverageMap_LookbackWindowWidth asserts that the CE TimePeriod
+// sent to GetReservationCoverage spans exactly lookbackDays calendar days.
+// This test is deliberately discriminating: it parses the YYYY-MM-DD Start
+// and End strings and verifies end-start == N days so a regression in the
+// window math (e.g. reverting to a hardcoded 30) causes a test failure.
+func TestGetRICoverageMap_LookbackWindowWidth(t *testing.T) {
+	cases := []struct {
+		name        string
+		lookbackDays int
+	}{
+		{"14-day window", 14},
+		{"60-day window", 60},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			mock := &mockCoverageCE{coverageOutput: &costexplorer.GetReservationCoverageOutput{}}
+			client := NewClientWithAPI(mock, "us-east-1")
+
+			before := time.Now().UTC()
+			_, err := client.GetRICoverageMap(context.Background(), tc.lookbackDays, []string{"us-east-1"})
+			after := time.Now().UTC()
+			require.NoError(t, err)
+			require.NotNil(t, mock.lastTimePeriod, "GetReservationCoverage must have been called with a TimePeriod")
+			require.NotNil(t, mock.lastTimePeriod.Start)
+			require.NotNil(t, mock.lastTimePeriod.End)
+
+			start, err := time.Parse("2006-01-02", aws.ToString(mock.lastTimePeriod.Start))
+			require.NoError(t, err, "TimePeriod.Start must be a valid YYYY-MM-DD date")
+			end, err := time.Parse("2006-01-02", aws.ToString(mock.lastTimePeriod.End))
+			require.NoError(t, err, "TimePeriod.End must be a valid YYYY-MM-DD date")
+
+			// CE dates are truncated to day; allow for midnight-boundary
+			// skew of at most 1 day when the test runs near UTC midnight.
+			spanDays := int(end.Sub(start).Hours() / 24)
+			assert.Equal(t, tc.lookbackDays, spanDays,
+				"CE TimePeriod must span exactly lookbackDays days (start=%s end=%s)",
+				aws.ToString(mock.lastTimePeriod.Start),
+				aws.ToString(mock.lastTimePeriod.End))
+
+			// Sanity: the End date must fall within the test's execution window.
+			endUTC := end
+			assert.True(t, !endUTC.Before(before.Truncate(24*time.Hour)),
+				"TimePeriod.End must not be before test start")
+			assert.True(t, !endUTC.After(after.Add(24*time.Hour)),
+				"TimePeriod.End must not be more than a day after test completion")
+		})
+	}
+}
+
 // TestApplyCoverageMapToRecommendations covers the org-wide pool matching:
 // recs look up by (region, instance_type, [engine, deployment]) so any
 // linked account in the org sees the same coverage % for the same pool

providers/aws/service_client.go

@@ -174,6 +174,12 @@ func (r *RecommendationsClientAdapter) GetRICoverageMap(ctx context.Context, loo
 	return r.client.GetRICoverageMap(ctx, lookbackDays, regions)
 }
 
+// SetRecLookbackPeriod configures the LookbackPeriodInDays forwarded to
+// GetReservationPurchaseRecommendation. Valid values: "7d", "30d", "60d".
+func (r *RecommendationsClientAdapter) SetRecLookbackPeriod(period string) {
+	r.client.SetRecLookbackPeriod(period)
+}
+
 // NewRecommendationsClientDirect creates a new recommendations client returning the concrete type
 // (needed for GetRIUtilization which is not part of the generic provider interface).
 func NewRecommendationsClientDirect(cfg aws.Config) *RecommendationsClientAdapter {