Skip to content

Align tests and schemas - #775

Open
hamistao wants to merge 7 commits into
rancher:mainfrom
hamistao:improve_test_schemas
Open

Align tests and schemas#775
hamistao wants to merge 7 commits into
rancher:mainfrom
hamistao:improve_test_schemas

Conversation

@hamistao

Copy link
Copy Markdown
Contributor

As part of a personal Goal, I experimented with using AI to identify discrepancies between our automated tests and their schemas and this PR is a result of trying to fix the identified discrepancies.

The more convoluted fixes here would be:

  • The helm install commands from TestAmbientInstallation and TestGatewayStandaloneInstallation schemas were updated to match the one used on the tests.
  • TestLoadBalancer and TestLoadBalanceScaleAndUpgrade were updated to test the connectivity to the actual Load Balancers instead of the Node port

@hamistao
hamistao requested a review from a team as a code owner July 22, 2026 16:57
@hamistao
hamistao requested a review from Copilot July 22, 2026 16:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to align PIT schemas with existing automated test behavior and to adjust load balancer connectivity tests to validate connectivity against the actual LoadBalancer endpoint rather than NodePort, reducing schema/test drift and improving real-world coverage.

Changes:

  • Refactors workload deployment test table setup to remove redundant per-test create function wiring.
  • Updates PIT schema steps/commands/positions across multiple suites to better match test execution.
  • Updates LoadBalancer tests to validate connectivity via the LoadBalancer service address and introduces a new helper for that validation.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
validation/workloads/workload_test.go Simplifies deployment test case struct by removing redundant create function field.
validation/workloads/schemas/pit_schemas.yaml Adjusts workload PIT schema formatting and scale-step definitions to match test behavior.
validation/upgrade/schemas/pit_schemas.yaml Updates PIT schema formatting and step position numbering for upgrade workload cases.
validation/networking/connectivity/port_test.go Switches LoadBalancer connectivity checks from NodePort to LoadBalancer service endpoint and simplifies test structure.
validation/longhorn/schemas/pit_schemas.yaml Updates Longhorn PIT schema steps to reflect revised replica/scale expectations (with a noted grammar fix needed).
validation/fleet/schemas/pit_schemas.yaml Updates expected namespace wording in Fleet PIT schema to match test expectation.
validation/charts/schemas/pit_schemas.yaml Updates chart PIT schema commands/details (alerting namespace, neuvector chart list) to match tests.
validation/charts/appco/schemas/pit_schemas.yaml Updates Istio AppCo standalone install schema command (currently diverges from test behavior regarding imagePullSecrets).
actions/workloads/deployment/verify.go Fixes replica update logging to reflect the computed target replica count.
actions/networking/verify.go Adds LoadBalancer connectivity helper (currently contains correctness/compilation issues that need fixes).

Comment thread actions/networking/verify.go
Comment thread actions/networking/verify.go
Comment thread validation/longhorn/schemas/pit_schemas.yaml Outdated
Comment thread validation/charts/appco/schemas/pit_schemas.yaml Outdated
Comment thread actions/networking/verify.go
Comment thread actions/networking/verify.go Outdated
hamistao added 4 commits July 22, 2026 14:58
Signed-off-by: hamistao <pedro.ribeiro@suse.com>
Signed-off-by: hamistao <pedro.ribeiro@suse.com>
Signed-off-by: hamistao <pedro.ribeiro@suse.com>
Signed-off-by: hamistao <pedro.ribeiro@suse.com>
@hamistao
hamistao force-pushed the improve_test_schemas branch from a1642af to dbe39dc Compare July 22, 2026 18:02
Signed-off-by: hamistao <pedro.ribeiro@suse.com>

@slickwarren slickwarren left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still need changes here; can you send a link to a test run using this?

Lastly can you confirm all callers of the changed signatures are updated:

VerifyClusterConnectivity changed its parameter from path string to port int. Please confirm there are no other callers of VerifyClusterConnectivity or VerifyNodePortConnectivity elsewhere in the repo that still use the old signature.

Comment thread actions/networking/verify.go Outdated
Comment thread actions/networking/verify.go Outdated
Comment thread validation/networking/connectivity/port_test.go Outdated
Signed-off-by: hamistao <pedro.ribeiro@suse.com>
@hamistao
hamistao force-pushed the improve_test_schemas branch from 59ac559 to f3f45c6 Compare July 28, 2026 11:05
lscalabrini01
lscalabrini01 previously approved these changes Jul 28, 2026
Comment thread actions/networking/verify.go Outdated
Comment thread actions/networking/verify.go Outdated
Comment thread actions/networking/verify.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (7)

actions/networking/verify.go:155

  • Go error strings should be lower-case. This error message currently starts with a capital letter.
		return fmt.Errorf("Curl result %s doesn't include the workload name %s", log, workloadName)

actions/networking/verify.go:190

  • This assumes the LoadBalancer ingress always has an IP (Status.LoadBalancer.Ingress[0].IP). On some providers it is only a hostname, so ip can be empty and the curl target becomes invalid. Also, the newly introduced error strings should be lower-case to follow Go conventions.
	port := k8sService.Spec.Ports[0].Port
	ip := k8sService.Status.LoadBalancer.Ingress[0].IP
	logrus.Infof("Testing connectivity with load balancer %s by curling %s:%d/name.html", k8sService.Name, ip, port)

validation/networking/connectivity/port_test.go:443

  • Spelling/terminology: this skip message refers to "Load Balance" but the test is for a Kubernetes Service of type "LoadBalancer".
		p.T().Skip("Load Balance test requires access to cloud provider.")

actions/networking/verify.go:121

  • When ip is empty, the code overwrites the ip parameter inside the loop. That makes subsequent iterations reuse the first resolved node IP, which contradicts the function comment (“Each node's own ip is used as default”) and can make NodePort checks flaky in environments where a node’s external IP isn’t routable from other worker nodes. Use a per-iteration targetIP instead of mutating ip.
		if ip == "" {
			newNode := &corev1.Node{}
			err = v1.ConvertToK8sType(machine.JSONResp, newNode)
			if err != nil {
				return fmt.Errorf("failed to convert node %s: %w", machine.Name, err)

actions/networking/verify.go:144

  • Go error strings should be lower-case and not start with a capital letter. This returned error message should be lower-cased for consistency with the rest of the codebase (e.g. "no worker nodes found").

This issue also appears on line 155 of the same file.

	return fmt.Errorf("Unable to connect to %s:%d/name.html from any worker node", ip, port)

actions/networking/verify.go:162

  • The function header comment is incomplete ("This includes") and doesn't add useful information. Please either remove the fragment or replace it with a complete description.
// VerifyLoadBalancerConnectivity verifies that the Load Balancer service is accessible by curling its IP:port.
// This includes

validation/networking/connectivity/port_test.go:198

  • Spelling/terminology: this skip message refers to "Load Balance" but the test is for a Kubernetes Service of type "LoadBalancer".

This issue also appears on line 443 of the same file.

		p.T().Skip("Load Balance test requires access to cloud provider.")

Signed-off-by: hamistao <pedro.ribeiro@suse.com>
@hamistao
hamistao force-pushed the improve_test_schemas branch from 7ca0f60 to e20cc9e Compare July 29, 2026 20:32
@slickwarren
slickwarren self-requested a review July 29, 2026 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants