-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: Improve e2e tests #4631
test: Improve e2e tests #4631
Conversation
e2e/cluster.go
Outdated
return VNet{}, fmt.Errorf("failed to find aks vnet") | ||
} | ||
|
||
func collectGarbageVMSS(ctx context.Context, cluster *armcontainerservice.ManagedCluster) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the pointer!
Model *armcontainerservice.ManagedCluster | ||
Kube *Kubeclient | ||
SubnetID string | ||
NodeBootstrappingConfiguration *datamodel.NodeBootstrappingConfiguration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is new
Pull Request Test Coverage Report for Build 9966466119Details
💛 - Coveralls |
e2e/scenario_test.go
Outdated
}) | ||
} | ||
|
||
func getContainerdManifestVersion() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to helpers?
e2e/vmss.go
Outdated
return nil, fmt.Errorf("begin create VMSS %q: %w", vmssName, err) | ||
var respErr *azcore.ResponseError | ||
// sometimes the SKU is not available and we can't do anything. Skip the test in this case. | ||
if config.SkipTestsWithSKUCapacityIssue && errors.As(err, &respErr) && respErr.StatusCode == 409 && respErr.ErrorCode == "SkuNotAvailable" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extract logic to check for SkuNotAvailable
to a helper to make this a bit more readable?
nbc.ContainerdVersion = "1.6.9" | ||
}, | ||
LiveVMValidators: []*LiveVMValidator{ | ||
containerdVersionValidator(getContainerdManifestVersion()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I didn't change this test yet. Now the containerd version should always come from components.json. I haven't removed the containerd version from manifest.json yet.
Feel free to change it or I will do that after your PR get merged.
nbc.AgentPoolProfile.Distro = "aks-azurelinux-v2-arm64-gen2" | ||
nbc.IsARM64 = true | ||
}, | ||
VMConfigMutator: func(vmss *armcompute.VirtualMachineScaleSet) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe in a follow-up you can rename VMConfigMutator
to VMSSModelMutator
to make it a bit more clear?
nbc.AgentPoolProfile.Distro = "aks-azurelinux-v2-gen2" | ||
}, | ||
LiveVMValidators: []*LiveVMValidator{ | ||
serviceCanRestartValidator("chronyd", 10), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe something else you can look at, let's have validators either consistently be exported or unexported, not a mixture of both
fine with just having them be unexported since we've moved them to the e2e
package at this point
nbc.ContainerService.Properties.AgentPoolProfiles[0].Distro = "aks-ubuntu-containerd-22.04-gen2" | ||
nbc.AgentPoolProfile.VMSize = vmSize | ||
nbc.AgentPoolProfile.Distro = "aks-ubuntu-containerd-22.04-gen2" | ||
nbc.ConfigGPUDriverIfNeeded = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another example I think where helpers would be beneficial for ease of test authorship,
e.g. WithGPUDriverInstallation
Better integration with IDE. Easier to debug a single test
![image](https://private-user-images.githubusercontent.com/2599261/348608922-a75d4cfd-78c3-473b-9b4f-f84da967633d.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ2MDQ0NTIsIm5iZiI6MTcyNDYwNDE1MiwicGF0aCI6Ii8yNTk5MjYxLzM0ODYwODkyMi1hNzVkNGNmZC03OGMzLTQ3M2ItOWI0Zi1mODRkYTk2NzYzM2QucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDgyNSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA4MjVUMTY0MjMyWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZDJjZTY1YzlkMDEwZDJiYmVlNjRkZGM4MjE0MjEzMDY3ZGE5ZmQ2YmIwMDlhNmU1MDI5NTk5MTA2NTVhYWNhZiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.E53Zk2WU_wjogH28ET-K_QfhM5wtd4a3h9D-mPr71cU)
Easier to follow the test logic. Execution sequence is more straightforward.
t.Log
instead oflog.Print
. Group logs by test, helps with log mixing due to parallel test execution