Skip to content

test: lifecycle/error-path coverage + teardown determinism (#68) - #81

Merged
jrse merged 2 commits into
feat/66-ci-baselinefrom
feat/68-lifecycle-coverage
Sep 24, 2026
Merged

jrse merged 2 commits into
feat/66-ci-baselinefrom
feat/68-lifecycle-coverage

Conversation

@senolcolak

Copy link
Copy Markdown
Collaborator

Closes #68. Fixes #78. Stacked on #80 (#66) → #79 (#67).

What

Adds fast, pure-unit coverage for the safety-critical reconciler branches that were previously reachable only through the slow envtest suite, fixes the teardown race that forced RandomizeAllSpecs off (#78), and corrects three webhook validation bugs that the new tests surfaced.

Coverage (#68)

  • Extract pure helpers allocateMonID and determinePublicAddressFor from the reconciler methods; the methods now delegate to these free functions (behavior-preserving — reviewed branch-by-branch). Unit-tested in pkg/controller/allocation_test.go (no build tag, no API server):
    • mon-ID: first-free, single/run collision skip, other-prefix isolation, last-suffix, a–z exhaustion error.
    • public address: nil-service pod-IP substitution, ClusterIP allocated/unallocated, NodePort, LoadBalancer IPv4 / skip-bad-then-IPv4 / no-ingress / IPv6-only, unknown type.
  • docs/testing/traceability.md: maps each reconcile transition and error branch to its covering spec, marking honest gaps for the deferred deletion-cascade and observedGeneration work (tracked follow-up).
  • Makefile: run ./pkg/controller/... in the unit layer (untagged pure tests), and add a report-only test-cover target merging unit + envtest coverage profiles (COVER_MIN gates once a floor is measured).

Teardown determinism (#78)

  • namespaceCleanUp force-strips finalizers tolerating both NotFound and Conflict, re-listing fresh each pass, converging inside a single Eventually that requires source and target namespaces empty in the same pass (source first, so the deleted RemoteArbiter stops repopulating the target).
  • Re-enable RandomizeAllSpecs. The "should succeed" spec waits 2 min: the reconciler has no watch on the target Deployment and only re-observes via the 1-min CheckInterval requeue, so the shared 30s timeout was intrinsically racy — this fixes the root cause, not the symptom.

Webhook fixes (regression-guarded)

Three field.Invalid calls reported the wrong value as BadValue, producing misleading admission errors:

  • cephCluster.namespace reported .Name; monIdPrefix reported remoteCluster.Name.
  • The NodePort nodeIp branch reported Service.Type and, on an unparseable IP, fell through to the Is4() check and appended a second spurious error. Fixed to one error naming the NodeIP.

Guarded by TestValidateRemoteArbiterSpecErrorBadValue and TestValidateRemoteArbiterSpecNodeIPErrorBadValue — both proven to fail on the pre-fix code and pass after.

Verification

gofmt clean · go vet ./... and -tags envtest clean · make test-unit green · golangci-lint 0 issues · envtest suite green (5× with shuffle) · make gen/helm/imports no-op.

@senolcolak
senolcolak force-pushed the feat/68-lifecycle-coverage branch from 0e9e573 to 0c264e0 Compare September 3, 2026 14:42
@senolcolak
senolcolak force-pushed the feat/68-lifecycle-coverage branch from 0c264e0 to 3d44052 Compare September 3, 2026 15:05
@senolcolak
senolcolak force-pushed the feat/68-lifecycle-coverage branch from 3d44052 to 7051131 Compare September 4, 2026 11:37
Adds fast pure-unit coverage for the safety-critical reconciler branches that
were previously reachable only through the slow envtest suite, fixes the
teardown race that forced RandomizeAllSpecs off (#78), and corrects three
webhook validation bugs surfaced by the new tests.

Coverage:
- Extract pure helpers allocateMonID and determinePublicAddressFor from the
  reconciler methods and unit-test every branch (mon-ID collision/exhaustion;
  all Service types + unallocated-IP/IPv6-only/no-ingress/unknown-type errors).
  Behavior-preserving: methods now delegate to these free functions.
- docs/testing/traceability.md maps each reconcile transition/error branch to
  its covering spec, marking honest gaps for the deferred deletion-cascade and
  observedGeneration work.
- Makefile: run ./pkg/controller/... in the unit layer (untagged pure tests),
  and add a report-only test-cover target that merges the unit and envtest
  coverage profiles (COVER_MIN gates once a floor is measured).

- namespaceCleanUp force-strips finalizers tolerating both NotFound and
  Conflict and re-lists fresh each pass, converging inside a single Eventually
  that requires source and target namespaces empty in the same pass (source
  first, so the deleted RemoteArbiter stops repopulating the target).
- Re-enable RandomizeAllSpecs. The "should succeed" spec waits 2 min: the
  reconciler has no watch on the target Deployment and only re-observes via the
  1-min CheckInterval requeue, so the shared 30s timeout was intrinsically racy.

Webhook fixes (regression-guarded):
- Two field.Invalid calls reported the wrong field's value as BadValue
  (cephCluster.namespace reported .Name; monIdPrefix reported remoteCluster.Name).
- The NodePort nodeIp branch reported Service.Type as BadValue and, on an
  unparseable IP, fell through to the Is4 check and appended a second spurious
  error. Fixed to one error naming the NodeIP.

Closes #68
Refs #78

Signed-off-by: senol.colak <senol.colak@sap.com>
…ion works

createArbiterService never set Spec.Type on the created Service, so a spec
requesting service.type=NodePort (or LoadBalancer) silently got a ClusterIP
Service. determinePublicAddressFor then picked the ClusterIP branch and baked
an unroutable in-cluster address into the monitor's --public-addr and the
monmap, with no error and no condition set.

Set the created Service's type from spec.service.type, and make the NodePort
branch of determinePublicAddressFor error on an empty node IP (mirroring the
ClusterIP/LoadBalancer unallocated-address branches) instead of returning an
empty address. Add a pure-unit regression row proving the empty-NodeIP guard.

Correct docs/testing/traceability.md: drop stale line anchors (function names
only), promote error-branch rows that were mislabeled 'partial' to covered
after verifying the specs assert the specific condition + Error state, and
record the Service-type propagation coverage honestly.

Signed-off-by: senol.colak <senol.colak@sap.com>
@senolcolak
senolcolak force-pushed the feat/68-lifecycle-coverage branch from 7051131 to b31f54e Compare September 4, 2026 11:46
@jrse
jrse self-requested a review September 9, 2026 07:01
@jrse

jrse commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

LGTM

@jrse
jrse merged commit abcb747 into feat/66-ci-baseline Sep 24, 2026
6 checks passed
@jrse
jrse deleted the feat/68-lifecycle-coverage branch September 24, 2026 08:18
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.

3 participants