Minor improvements to release test framework #946

pau-hedgehog · 2025-09-08T22:14:21Z

Cosmetic improvements to make release tests more readable to human beings

Copilot

Pull Request Overview

This PR implements comprehensive test progress tracking and enhanced logging for the release test framework. The changes improve user experience by showing test execution progress and providing detailed environment information before test runs.

Adds progress tracking with current test number and total test count across all suites
Implements detailed environment logging including device lists, component versions, and test execution plans
Refactors test execution with improved logging and duration tracking for individual tests

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

pkg/hhfab/release.go

Signed-off-by: Pau Capdevila <[email protected]>

edipascale · 2025-09-10T07:41:21Z

pkg/hhfab/release.go

 	testCtx.pauseOnFail = rtOpts.PauseOnFail
 	testCtx.roceLeaves = roceLeaves

+	// Initialize progress tracking


nit: you don't need to initialize these to 0, as it is the default value for integers

edipascale · 2025-09-10T07:48:34Z

pkg/hhfab/release.go

+		suiteRunnable := 0
+		suiteSkipped := 0
+
+		for _, test := range suite.TestCases {


We really shouldn't implement the skipping and selecting logic twice. Instead, please extract the current selection code from the selectAndRunSuite() function, which you can then call here; when we are ready to run them you can just call doRunTests() on the already selected suites.

edipascale · 2025-09-10T07:50:08Z

pkg/hhfab/release.go

+			case test.Skipped != nil:
+				slog.Warn("  SKIP", "test", test.Name, "reason", test.Skipped.Message)
+			case test.Failure != nil:
+				slog.Error("  FAIL", "test", test.Name, "error", strings.Split(test.Failure.Message, "\n")[0])


this has been changed in master to print all of the errors and replace newlines with semicolons, please update this from the existing code

edipascale · 2025-09-10T07:52:18Z

pkg/hhfab/release.go

+
+		testCtx.currentTestNum++
+		progress := ""
+		if testCtx.totalTests > 0 {


how can this ever be 0 if we get here?

edipascale · 2025-09-10T07:57:29Z

pkg/hhfab/release.go

+
+		slog.Info("* Running test", "progress", progress, "test", test.Name, "suite", ts.Name)
+
+		if ((testCtx.currentTestNum > 1) && testCtx.wipeBetweenTests) || prevRevertsFailed {


This logic is not equivalent to the previous one. Previously ranSomeTests was scoped to a single suite, also because we always reset the VPCs before any suite (except noVPCs one which runs first anyway). currentTestNum however will be always > 1 after we passed the first test, regardless of a new suite. This will needlessly rerun setup-vpcs if this is say our second suite and we skipped the first test of that suite

edipascale · 2025-09-10T08:04:13Z

pkg/hhfab/release.go

 	noVpcTestCtx.noSetup = true
-	noVpcSuite := makeNoVpcsSuiteRun(noVpcTestCtx)
-	noVpcResults, err := selectAndRunSuite(ctx, noVpcTestCtx, noVpcSuite, regexesCompiled, rtOpts.InvertRegex, skipFlags)
+	noVpcTestCtx.totalTests = totalRunnableTests


I should probably refactor this code as there's no real reason to create a new testCtx every time, we are only changing a handful of params... but until we do that, can we add these new variables as parameters to makeTestCtx, so we don't have to update them manually every time?

Frostman · 2025-09-10T16:11:05Z

pkg/hhfab/release.go

+		case sw.Spec.Role.IsLeaf():
+			leaves = append(leaves, sw.Name)
+		default:
+			// For any other roles, we can check the string representation


There are no other roles possible then the ones defined by SwitchRole - https://github.com/githedgehog/fabric/blob/83d69b2d4de3e2c3a01d2f201ac5e14ced2cecf6/api/wiring/v1beta1/switch_types.go#L38-L43 so please use it instead of contains check

Frostman · 2025-09-10T16:11:29Z

pkg/hhfab/release.go

+			// For any other roles, we can check the string representation
+			roleStr := string(sw.Spec.Role)
+			if strings.Contains(strings.ToLower(roleStr), "border") {
+				borders = append(borders, sw.Name)


it's most probably going to be mixed always, we don't have dedicated borders in any envs

Frostman · 2025-09-10T16:11:38Z

pkg/hhfab/release.go

+			roleStr := string(sw.Spec.Role)
+			if strings.Contains(strings.ToLower(roleStr), "border") {
+				borders = append(borders, sw.Name)
+			} else if strings.Contains(strings.ToLower(roleStr), "mgmt") {


We don't have a concept of mgmt switch

Frostman · 2025-09-10T16:12:16Z

pkg/hhfab/release.go

+	borders := []string{}
+	mgmt := []string{}
+
+	for _, sw := range swList.Items {


It'll be probably better just to iterate over the list of switches and print them one by one with the role and any other metadata needed and then the same things for the servers/etc, but to be honest why do we need that? We have it all in the bundle for the job with versions/wirings/etc

but to be honest why do we need that?

I think it is useful to have a quick summary of the system under test, in case the tester missed something and would have to redeploy/restart/retest. IMO The wiring is too much info to review each time and you could miss something not obvious

Frostman · 2025-09-10T16:16:08Z

pkg/hhfab/release.go

+	slog.Info("───────────────────────────────────────────────────────────────────────────")
+
+	// Component Versions
+	slog.Info("Component Versions:")


That code will become outdated very quickly, we already have versions.txt in the CI bundle, and if you run it manually you can always run hhfab versions instead of having a code that'll go stale quickly.

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-30T20:55:40Z

pkg/hhfab/release.go

+	noVpcSuite := makeNoVpcsSuiteRun(&VPCPeeringTestCtx{})
+	singleVpcSuite := makeVpcPeeringsSingleVPCSuite(&VPCPeeringTestCtx{})
+	multiVpcSuite := makeVpcPeeringsMultiVPCSuiteRun(&VPCPeeringTestCtx{})
+	basicVpcSuite := makeVpcPeeringsBasicSuiteRun(&VPCPeeringTestCtx{})


[nitpick] Creating empty test contexts just for suite planning creates unnecessary instances. Consider extracting the test case definitions into separate functions that don't require a test context, or add a parameter to the make functions to indicate they're being used for planning only.

Copilot · 2025-09-30T20:55:40Z

pkg/hhfab/release.go

+		for i := 0; i < len(controls); i++ {
+			slog.Info(fmt.Sprintf("    control-%d: control", i))


[nitpick] The loop uses a generic index i and constructs a generic name control-i. Consider using a more descriptive approach or iterating over the actual control node names if available.

Suggested change

for i := 0; i < len(controls); i++ {

slog.Info(fmt.Sprintf(" control-%d: control", i))

for _, control := range controls {

slog.Info(fmt.Sprintf(" %s: control", control.Name))

pau-hedgehog requested a review from Copilot September 8, 2025 22:14

pau-hedgehog self-assigned this Sep 8, 2025

pau-hedgehog added ci:-upgrade Disable VLAB upgrade tests ci:+release Enable VLAB release tests ci:+hlab Enable hybrid VLAB tests labels Sep 8, 2025

Copilot AI reviewed Sep 8, 2025

View reviewed changes

pkg/hhfab/release.go Show resolved Hide resolved

pkg/hhfab/release.go Outdated Show resolved Hide resolved

pkg/hhfab/release.go Outdated Show resolved Hide resolved

pkg/hhfab/release.go Outdated Show resolved Hide resolved

pau-hedgehog force-pushed the pau/release_test_dx branch from 4265324 to 89895e8 Compare September 8, 2025 23:16

pau-hedgehog changed the title ~~Pau/release test dx~~ Minor improvements to release test framework Sep 9, 2025

pau-hedgehog force-pushed the pau/release_test_dx branch from 89895e8 to ebba4a4 Compare September 9, 2025 18:28

pau-hedgehog added 4 commits September 10, 2025 09:15

chore(testing): display release test session startup summary

79854dc

Signed-off-by: Pau Capdevila <[email protected]>

chore(testing): fix typo in Release Test Options struct name

861c822

Signed-off-by: Pau Capdevila <[email protected]>

chore(testing): track test run progress and refactor result summary

e3b50a7

Signed-off-by: Pau Capdevila <[email protected]>

chore(testing): extend test session initial summary

7ea9dba

Signed-off-by: Pau Capdevila <[email protected]>

pau-hedgehog force-pushed the pau/release_test_dx branch from ebba4a4 to 7ea9dba Compare September 10, 2025 07:15

pau-hedgehog marked this pull request as ready for review September 10, 2025 07:15

pau-hedgehog requested a review from a team as a code owner September 10, 2025 07:15

pau-hedgehog requested a review from edipascale September 10, 2025 07:15

edipascale requested changes Sep 10, 2025

View reviewed changes

Frostman requested changes Sep 10, 2025

View reviewed changes

pau-hedgehog removed the ci:+hlab Enable hybrid VLAB tests label Sep 15, 2025

pau-hedgehog marked this pull request as draft September 19, 2025 10:25

Frostman requested a review from Copilot September 30, 2025 20:54

Copilot AI reviewed Sep 30, 2025

View reviewed changes


		slog.Info("* Running test", "progress", progress, "test", test.Name, "suite", ts.Name)

		if ((testCtx.currentTestNum > 1) && testCtx.wipeBetweenTests) \|\| prevRevertsFailed {

		for i := 0; i < len(controls); i++ {
		slog.Info(fmt.Sprintf(" control-%d: control", i))

Uh oh!

Minor improvements to release test framework #946

Are you sure you want to change the base?

Minor improvements to release test framework #946

Uh oh!

Conversation

pau-hedgehog commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pau-hedgehog commented Sep 8, 2025 •

edited

Loading