Re-enable controller test suite and harden scheme registration#48
Merged
Conversation
![kodiakhq[bot]](https://avatars.githubusercontent.com/in/29196?s=60&v=4){kind=small}
tamalsaha
force-pushed
the
fix-tests-and-review
branch
from
db2a354 to
2a88d10
Compare
The ported upstream StatefulSet controller tests in pkg/controller/tests/ were entirely commented out (~6500 lines) and lived in a package that could not access the unexported identifiers they test, so none of them ran. Move the five test files into pkg/controller/petset/ as white-box tests and adapt them to the fork's API drift: - Update the fakeObjectManager to the current StatefulPodControlObjectManager interface (PetSet params, ListPods, GetPlacementPolicy). - Add thin test adapters for newPetSetPod and NewStatefulPodControl, and wire the PlacementPolicy informer plus a fake OCM ManifestWork client/informer into newFakePetSetController and NewPetSetController. - Point feature-gate tests at the fork's features.DefaultFeatureGate and the new SetFeatureGateDuringTest signature. - Restore lost helpers (ascendingOrdinal, overlappingPetSets). Fix bugs surfaced while re-enabling the tests: - setupController listed StatefulSets from the kube client instead of PetSets from the api client, so the set never entered the indexer. - TestPetSetControl_getSetRevisions waited on a kube StatefulSets informer that was instantiated after Start() and never synced, hanging the suite. - newFakePetSetController seeded CRD objects into the core kube fake client, which panics; route PetSet/PlacementPolicy to the versioned fake. Harden patchCodec: register the PetSet/PlacementPolicy types with the client-go scheme in an init() next to patchCodec instead of relying on a PersistentPreRunE hook in cmd wiring. Otherwise constructing the controller through any other path panics with "no kind is registered for the type v1.PetSet" the first time a ControllerRevision is created. Add placement_test.go covering the AppsCode placement logic: the upsert helpers, spread/anti-affinity and node-affinity generation, domain selection in getAppropriateDomainIndex, and the CEL evaluator with its program cache. Signed-off-by: Tamal Saha <tamal@appscode.com>
Restore the upstream pkg/controller/controller_utils_test.go that was dropped in c547220 ("Use golangci-lint 2.x"). All the helpers it exercises still exist in the fork, so the coverage is worth keeping. Adapt it to the fork and the vendored dependency set: - Swap k8s.io/kubernetes/test/utils/ktesting for k8s.io/klog/v2/ktesting (identical NewTestContext API, and the former is not vendored). - Point feature-gate tests at features.DefaultFeatureGate and the new SetFeatureGateDuringTest signature. - ComputeHash now takes *api.PodTemplateSpec; update TestComputeHash. - gofmt interface{} -> any to satisfy the repo formatter config. Drop the tests that would require pulling additional heavy test-only deps into vendor for little benefit: - TestRemoveTaintOffNode / TestAddOrUpdateTaintOnNode need k8s.io/kubernetes/pkg/controller/testutil (FakeNodeHandler); the taint helpers they cover are unused in this fork. - TestCreatePodsWithGenerateName needs k8s.io/client-go/util/testing (FakeHandler) plus a rewrite to the PodInfo-based CreatePods signature. Vendor k8s.io/utils/clock/testing for the fake clock used by the ControllerExpectations tests. Relax errcheck/unparam on _test.go files in .golangci.yml: the revived controller tests mirror the upstream Kubernetes StatefulSet controller tests, which freely ignore errors from in-memory fake stores and use helper signatures that trip unparam; excluding test files keeps the suite rebasable against upstream while production code stays strictly linted. Signed-off-by: Tamal Saha <tamal@appscode.com>
tamalsaha
force-pushed
the
fix-tests-and-review
branch
from
2a88d10 to
9ecabb3
Compare
make ci was check-license lint build with unit-tests/cover/verify commented out, so CI ran no tests at all. Now that the controller suite is re-enabled and race-clean under -race, add unit-tests to the ci target so the suite actually guards changes. (cover/verify remain opt-in.) Signed-off-by: Tamal Saha <tamal@appscode.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The ported upstream StatefulSet controller tests in
pkg/controller/tests/were entirely commented out (~6,500 lines across 5 files) and declared a package that could not reach the unexported identifiers they exercise, sogo testreported[no tests to run]and 0 of 87 controller test functions ever ran. This PR re-enables them, fixes the bugs they surface, hardens a fragile production scheme registration, and adds first-time coverage for the AppsCode placement logic.Re-enable the controller test suite
Moved the five files into
pkg/controller/petset/as white-box tests and adapted them to the fork's API drift:fakeObjectManagerto the currentStatefulPodControlObjectManagerinterface (PetSet params,ListPods,GetPlacementPolicy).newPetSetPodandNewStatefulPodControl, and wired thePlacementPolicyinformer plus a fake OCMManifestWorkclient/informer intonewFakePetSetController.features.DefaultFeatureGateand the newSetFeatureGateDuringTestsignature.ascendingOrdinal,overlappingPetSets).Bugs surfaced while re-enabling
setupControllerlistedStatefulSetsfrom the kube client (always empty) instead ofPetSetsfrom the api client, so the set never entered the indexer.TestPetSetControl_getSetRevisionswaited on a kubeStatefulSetsinformer instantiated afterStart()that never synced — hanging the whole suite to the 10‑minute timeout.newFakePetSetControllerseeded CRD objects into the core kube fake client (which panics); CRD objects are now routed to the versioned fake.Harden scheme registration
patchCodec(pkg/controller/petset/pet_set_utils.go) serializes PetSets into ControllerRevisions via the client-go global scheme, but the type was only registered as a side effect inpkg/cmds/root.go'sPersistentPreRunE. Any path that builds the controller differently panics with "no kind is registered for the type v1.PetSet" the first time a revision is created — exactly what broke the tests. Registration now happens in aninit()next topatchCodec(same package, so it always runs before use), and the redundant cmd-layer hook is removed.New:
placement_test.goFirst coverage for the AppsCode-specific placement logic: the upsert helpers, spread/anti-affinity and node-affinity generation, domain selection in
getAppropriateDomainIndex, and the CEL evaluator including its program cache.Verification
go build ./...,go vet ./...,gofmt, andgo test ./...all pass. Thepkg/controller/petsetpackage now runs all 87 previously-disabled tests plus the new placement tests.