test(computing-unit): cover the pod lookup, creation and deletion paths - #7511
Merged
Conversation
KubernetesClientSpec covered the pure fabric8-to-map transforms and the namespace-wide wrappers, and stopped there. The single-pod half of the class - getPodByName, podExists, getPodLimits, createPod, deletePod and the pod URI - was untested, which is 42 of the file's 62 lines. None of it needs a cluster. The fabric8 client is already a constructor parameter, and the existing spec's Mockito plumbing extends to the rest of the fluent chain: withName(...).get() for the lookups and resource(pod).inNamespace(...).create() for creation, with the built pod captured and inspected. Adds 8 tests. The ones worth having are the guard that refuses to overwrite a live pod, the null-to-None wrapper that fabric8 requires, and the shared-memory volume appearing only when a size is requested. No production file is touched.
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
computing-unit-managing-service/src/test/scala/org/apache/texera/service/util/KubernetesClientSpec.scala:298
- In the shared-memory helper, the pod argument is captured via
verify(client).resource(...), but the test doesn't assert that the fluent chain actually reaches.create(). Adding averify(resource).create()makes the intent (“create the pod”) explicit and protects against regressions where creation is skipped.
when(client.resource(any(classOf[Pod]))).thenReturn(namespaceable)
when(namespaceable.inNamespace(namespace)).thenReturn(resource)
// create()'s return value is not asserted; the pod is inspected through the captor below.
when(resource.create()).thenReturn(null)
new KubernetesClient(client).createPod(6, "1", "2Gi", "0", Map.empty, shm)
verify(client).resource(captor.capture())
captor.getValue
computing-unit-managing-service/src/test/scala/org/apache/texera/service/util/KubernetesClientSpec.scala:271
- This test captures the pod passed to
client.resource(...)but doesn't assert thatcreate()was actually invoked. IfcreatePodwere to accidentally stop calling.create()(e.g., returning the built pod without creating it), this test would still pass.
Consider verifying the terminal call on the fluent chain as well (and optionally inNamespace(namespace)), to ensure the method truly performs creation.
This issue also appears on line 292 of the same file.
when(client.resource(any(classOf[Pod]))).thenReturn(namespaceable)
when(namespaceable.inNamespace(namespace)).thenReturn(resource)
// create()'s return value is not asserted; the pod is inspected through the captor below.
when(resource.create()).thenReturn(null)
new KubernetesClient(client).createPod(5, "2", "4Gi", "1", Map("UID" -> 9, "MODE" -> "batch"))
verify(client).resource(captor.capture())
val built = captor.getValue
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7511 +/- ##
============================================
- Coverage 86.52% 86.47% -0.06%
- Complexity 4211 4221 +10
============================================
Files 1169 1169
Lines 46754 46750 -4
Branches 5203 5203
============================================
- Hits 40454 40427 -27
- Misses 4577 4599 +22
- Partials 1723 1724 +1
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
What changes were proposed in this PR?
The spec covered the pure transforms and the namespace-wide wrappers and stopped. The single-pod
half of the class —
getPodByName,podExists,getPodLimits,createPod,deletePodand thepod URI — was untested, which is 42 of the file's 62 lines.
None of it needs a cluster: the fabric8 client is already a constructor parameter, so the existing
Mockito fixture extends to the rest of the fluent chain, with the pod that
createPodbuildscaptured and inspected rather than sent anywhere.
Adds 8 tests. The three that matter:
from its owner;
Option(...)wrapper around the by-name lookup — fabric8 returnsnullfor an absentpod rather than throwing, so the wrapper is all that stands between a caller and an NPE;
/dev/shmdefaults to64 Mi, too small for the Python workers, and the volume must not appear when unrequested.
Also covered: the pod URI's service and namespace segments, the first container's resource limits
and the empty-map fallback, env values reaching the container as strings, and
deletePodtargetingthe cuid's own pod.
Verified by mutation, all reverted (production diff empty):
getPodByNameno longer null-guardscreatePodoverwrites an existing poddeletePodtargets a fixed cuidNo production file is touched.
Any related issues, documentation, discussions?
Closes #7510
How was this PR tested?
8 new on top of the existing 8.
Test/scalafmtCheckandTest/scalafix --checkboth pass.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)