Align tests and schemas#775
Conversation
There was a problem hiding this comment.
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). |
|
|
||
| k8sService := &corev1.Service{} | ||
| err = steveV1.ConvertToK8sType(service, k8sService) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
We usually just return the error from the action and verify it in the test. As an example: https://github.com/hamistao/tests/blob/a1642af8087366191e6d61b758e7cf7ea600028e/actions/networking/verify.go#L49
There was a problem hiding this comment.
Are you suggesting that I remove t *testing.T from the parameters? I don't think it makes sense to take in the parameter and not use it here. The example you gave doesn't include such a parameter thus it makes sense for it to return the error.
I decided to include this parameter here so we could have the IP and port in the logging as well, which I think is nice.
There was a problem hiding this comment.
I can use logrus here instead if you prefer
There was a problem hiding this comment.
I haven't seen the t *testing.T, yes you can remove it and use the logrus
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>
a1642af to
dbe39dc
Compare
Signed-off-by: hamistao <pedro.ribeiro@suse.com>
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:
helm installcommands fromTestAmbientInstallationandTestGatewayStandaloneInstallationschemas were updated to match the one used on the tests.TestLoadBalancerandTestLoadBalanceScaleAndUpgradewere updated to test the connectivity to the actual Load Balancers instead of the Node port