Skip to content

Commit 574bc88

Browse files
sadasusadasu
committed
Address code review comments and fix unit tests
1 parent 3133041 commit 574bc88

2 files changed

Lines changed: 43 additions & 74 deletions

File tree

‎tools/codegen/cmd/featuregate-test-analyzer.go‎

Lines changed: 22 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package main
22

33
import (
44
"context"
5-
"crypto/tls"
65
"encoding/json"
76
"errors"
87
"fmt"
@@ -496,7 +495,7 @@ func checkIfTestingIsSufficient(featureGate string, testingResults map[JobVarian
496495
results = append(results, ValidationResult{
497496
Error: fmt.Errorf("error: \"install should succeed: overall\" test data not found for Install feature gate %q on %v",
498497
featureGate, jobVariant),
499-
IsWarning: false,
498+
IsWarning: isOptional,
500499
Category: CategoryInstallTest,
501500
})
502501
} else {
@@ -1061,27 +1060,27 @@ func getRelease() (string, error) {
10611060
return getLatestRelease()
10621061
}
10631062

1063+
func newSippyClient() *http.Client {
1064+
return &http.Client{
1065+
Timeout: 2 * time.Minute,
1066+
Transport: &http.Transport{
1067+
Proxy: http.ProxyFromEnvironment,
1068+
ForceAttemptHTTP2: true,
1069+
MaxIdleConns: 100,
1070+
IdleConnTimeout: 90 * time.Second,
1071+
TLSHandshakeTimeout: 10 * time.Second,
1072+
ExpectContinueTimeout: 1 * time.Second,
1073+
},
1074+
}
1075+
}
1076+
10641077
func getInstallTestResultsFromJobRuns(featureGate string, jobVariant JobVariant) (*TestResults, error) {
10651078
release, err := getRelease()
10661079
if err != nil {
10671080
return nil, fmt.Errorf("couldn't determine release: %w", err)
10681081
}
10691082

1070-
defaultTransport := &http.Transport{
1071-
Proxy: http.ProxyFromEnvironment,
1072-
ForceAttemptHTTP2: true,
1073-
MaxIdleConns: 100,
1074-
IdleConnTimeout: 90 * time.Second,
1075-
TLSHandshakeTimeout: 10 * time.Second,
1076-
ExpectContinueTimeout: 1 * time.Second,
1077-
TLSClientConfig: &tls.Config{
1078-
InsecureSkipVerify: true,
1079-
},
1080-
}
1081-
sippyClient := &http.Client{
1082-
Timeout: 2 * time.Minute,
1083-
Transport: defaultTransport,
1084-
}
1083+
sippyClient := newSippyClient()
10851084

10861085
jobs, err := getJobsForFeatureGateFromSippy(sippyClient, release, featureGate, jobVariant)
10871086
if err != nil {
@@ -1129,22 +1128,7 @@ func getInstallTestLevelData(featureGate string, jobVariant JobVariant) (*TestRe
11291128
queries := sippy.QueriesForWithCapability(jobVariant.Cloud, jobVariant.Architecture, jobVariant.Topology,
11301129
jobVariant.NetworkStack, jobVariant.OS, jobVariant.JobTiers, testPattern, featureGate)
11311130

1132-
defaultTransport := &http.Transport{
1133-
Proxy: http.ProxyFromEnvironment,
1134-
ForceAttemptHTTP2: true,
1135-
MaxIdleConns: 100,
1136-
IdleConnTimeout: 90 * time.Second,
1137-
TLSHandshakeTimeout: 10 * time.Second,
1138-
ExpectContinueTimeout: 1 * time.Second,
1139-
TLSClientConfig: &tls.Config{
1140-
InsecureSkipVerify: true,
1141-
},
1142-
}
1143-
1144-
sippyClient := &http.Client{
1145-
Timeout: 2 * time.Minute,
1146-
Transport: defaultTransport,
1147-
}
1131+
sippyClient := newSippyClient()
11481132

11491133
release, err := getRelease()
11501134
if err != nil {
@@ -1177,14 +1161,14 @@ func getInstallTestLevelData(featureGate string, jobVariant JobVariant) (*TestRe
11771161
if err != nil {
11781162
return nil, err
11791163
}
1164+
defer response.Body.Close()
11801165
if response.StatusCode < 200 || response.StatusCode > 299 {
11811166
return nil, fmt.Errorf("error getting sippy results (status=%d) for: %v", response.StatusCode, currURL.String())
11821167
}
11831168
queryResultBytes, err := io.ReadAll(response.Body)
11841169
if err != nil {
11851170
return nil, err
11861171
}
1187-
response.Body.Close()
11881172

11891173
testInfos := []sippy.SippyTestInfo{}
11901174
if err := json.Unmarshal(queryResultBytes, &testInfos); err != nil {
@@ -1231,22 +1215,7 @@ func listTestResultForVariant(featureGate string, jobVariant JobVariant) (*Testi
12311215
jobVariant.NetworkStack, jobVariant.OS, jobVariant.JobTiers, testPattern)
12321216
fmt.Printf("Query sippy for all test run results for pattern %q on variant %#v\n", testPattern, jobVariant)
12331217

1234-
defaultTransport := &http.Transport{
1235-
Proxy: http.ProxyFromEnvironment,
1236-
ForceAttemptHTTP2: true,
1237-
MaxIdleConns: 100,
1238-
IdleConnTimeout: 90 * time.Second,
1239-
TLSHandshakeTimeout: 10 * time.Second,
1240-
ExpectContinueTimeout: 1 * time.Second,
1241-
TLSClientConfig: &tls.Config{
1242-
InsecureSkipVerify: true,
1243-
},
1244-
}
1245-
1246-
sippyClient := &http.Client{
1247-
Timeout: 2 * time.Minute,
1248-
Transport: defaultTransport,
1249-
}
1218+
sippyClient := newSippyClient()
12501219

12511220
testNameToResults := map[string]*TestResults{}
12521221
hasCandidateTierResults := false
@@ -1280,14 +1249,14 @@ func listTestResultForVariant(featureGate string, jobVariant JobVariant) (*Testi
12801249
if err != nil {
12811250
return nil, err
12821251
}
1252+
defer response.Body.Close()
12831253
if response.StatusCode < 200 || response.StatusCode > 299 {
12841254
return nil, fmt.Errorf("error getting sippy results (status=%d) for: %v", response.StatusCode, currURL.String())
12851255
}
12861256
queryResultBytes, err := io.ReadAll(response.Body)
12871257
if err != nil {
12881258
return nil, err
12891259
}
1290-
response.Body.Close()
12911260

12921261
testInfos := []sippy.SippyTestInfo{}
12931262
if err := json.Unmarshal(queryResultBytes, &testInfos); err != nil {
@@ -1352,22 +1321,7 @@ func verifyJobBasedFeatureGatePromotion(featureGate string, jobVariant JobVarian
13521321
return nil, fmt.Errorf("getting release version: %w", err)
13531322
}
13541323

1355-
defaultTransport := &http.Transport{
1356-
Proxy: http.ProxyFromEnvironment,
1357-
ForceAttemptHTTP2: true,
1358-
MaxIdleConns: 100,
1359-
IdleConnTimeout: 90 * time.Second,
1360-
TLSHandshakeTimeout: 10 * time.Second,
1361-
ExpectContinueTimeout: 1 * time.Second,
1362-
TLSClientConfig: &tls.Config{
1363-
InsecureSkipVerify: true,
1364-
},
1365-
}
1366-
1367-
sippyClient := &http.Client{
1368-
Timeout: 2 * time.Minute,
1369-
Transport: defaultTransport,
1370-
}
1324+
sippyClient := newSippyClient()
13711325

13721326
jobs, err := getJobsForFeatureGateFromSippy(sippyClient, ocpRelease, featureGate, jobVariant)
13731327
if err != nil {
@@ -1559,6 +1513,7 @@ func getTriagedTestFailuresFromSippy(client *http.Client, release string, varian
15591513
if err != nil {
15601514
return nil, fmt.Errorf("getting sippy triages: %w", err)
15611515
}
1516+
defer resp.Body.Close()
15621517

15631518
if resp.StatusCode != http.StatusOK {
15641519
return nil, fmt.Errorf("expected a 200 OK status code but got %d", resp.StatusCode)
@@ -1569,8 +1524,6 @@ func getTriagedTestFailuresFromSippy(client *http.Client, release string, varian
15691524
return nil, fmt.Errorf("reading response body: %w", err)
15701525
}
15711526

1572-
defer resp.Body.Close()
1573-
15741527
triageItems := []sippy.SippyTriageItem{}
15751528
err = json.Unmarshal(body, &triageItems)
15761529
if err != nil {

‎tools/codegen/cmd/featuregate-test-analyzer_test.go‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ func Test_checkIfTestingIsSufficient_CandidateVariants(t *testing.T) {
241241

242242
for _, tt := range tests {
243243
t.Run(tt.name, func(t *testing.T) {
244-
results := checkIfTestingIsSufficient(tt.featureGate, tt.testingResults)
244+
results := checkIfTestingIsSufficient(tt.featureGate, tt.testingResults, nil)
245245

246246
blockingErrors := 0
247247
warnings := 0
@@ -449,7 +449,7 @@ func Test_checkIfTestingIsSufficient_OptionalVariants(t *testing.T) {
449449

450450
for _, tt := range tests {
451451
t.Run(tt.name, func(t *testing.T) {
452-
results := checkIfTestingIsSufficient(tt.featureGate, tt.testingResults)
452+
results := checkIfTestingIsSufficient(tt.featureGate, tt.testingResults, nil)
453453

454454
blockingErrors := 0
455455
warnings := 0
@@ -486,6 +486,7 @@ func Test_checkIfTestingIsSufficient_InstallFeatureGates(t *testing.T) {
486486
name string
487487
featureGate string
488488
testingResults map[JobVariant]*TestingResults
489+
installTestData map[JobVariant]*TestResults
489490
wantBlockingErrors int
490491
wantWarnings int
491492
}{
@@ -507,6 +508,9 @@ func Test_checkIfTestingIsSufficient_InstallFeatureGates(t *testing.T) {
507508
},
508509
},
509510
},
511+
installTestData: map[JobVariant]*TestResults{
512+
{Cloud: "metal", Architecture: "amd64", Topology: "ha"}: {TestName: "install should succeed: overall", TotalRuns: 20, SuccessfulRuns: 19},
513+
},
510514
wantBlockingErrors: 1, // Blocking error: install should succeed: overall must pass at 100%
511515
wantWarnings: 0,
512516
},
@@ -528,8 +532,9 @@ func Test_checkIfTestingIsSufficient_InstallFeatureGates(t *testing.T) {
528532
},
529533
},
530534
},
531-
wantBlockingErrors: 0,
532-
wantWarnings: 1, // Warning about missing "install should succeed: overall" test
535+
installTestData: map[JobVariant]*TestResults{},
536+
wantBlockingErrors: 1, // Blocking error: install test data not found on required variant
537+
wantWarnings: 0,
533538
},
534539
{
535540
name: "Non-Install feature gate with install should succeed: overall test - no special validation",
@@ -549,6 +554,7 @@ func Test_checkIfTestingIsSufficient_InstallFeatureGates(t *testing.T) {
549554
},
550555
},
551556
},
557+
installTestData: nil,
552558
wantBlockingErrors: 0,
553559
wantWarnings: 0,
554560
},
@@ -570,6 +576,9 @@ func Test_checkIfTestingIsSufficient_InstallFeatureGates(t *testing.T) {
570576
},
571577
},
572578
},
579+
installTestData: map[JobVariant]*TestResults{
580+
{Cloud: "aws", Architecture: "amd64", Topology: "ha"}: {TestName: "install should succeed: overall", TotalRuns: 20, SuccessfulRuns: 20},
581+
},
573582
wantBlockingErrors: 0,
574583
wantWarnings: 0,
575584
},
@@ -604,6 +613,10 @@ func Test_checkIfTestingIsSufficient_InstallFeatureGates(t *testing.T) {
604613
},
605614
},
606615
},
616+
installTestData: map[JobVariant]*TestResults{
617+
{Cloud: "metal", Architecture: "amd64", Topology: "ha"}: {TestName: "install should succeed: overall", TotalRuns: 20, SuccessfulRuns: 19},
618+
{Cloud: "metal", Architecture: "amd64", Topology: "single"}: {TestName: "install should succeed: overall", TotalRuns: 18, SuccessfulRuns: 18},
619+
},
607620
wantBlockingErrors: 1, // One variant (ha) fails 100% requirement
608621
wantWarnings: 0,
609622
},
@@ -625,14 +638,17 @@ func Test_checkIfTestingIsSufficient_InstallFeatureGates(t *testing.T) {
625638
},
626639
},
627640
},
641+
installTestData: map[JobVariant]*TestResults{
642+
{Cloud: "aws", Architecture: "amd64", Topology: "ha"}: {TestName: "install should succeed: overall", TotalRuns: 10, SuccessfulRuns: 10},
643+
},
628644
wantBlockingErrors: 1, // Blocking error for insufficient runs (< 14)
629645
wantWarnings: 0,
630646
},
631647
}
632648

633649
for _, tt := range tests {
634650
t.Run(tt.name, func(t *testing.T) {
635-
results := checkIfTestingIsSufficient(tt.featureGate, tt.testingResults)
651+
results := checkIfTestingIsSufficient(tt.featureGate, tt.testingResults, tt.installTestData)
636652

637653
blockingErrors := 0
638654
warnings := 0

0 commit comments

Comments
 (0)