Conversation
TestUninstallTerminatesRunningInstance asserts that Uninstall reclaims a
managed engine started without an executor process handle. That reclaim
needs the listener's executable path, and procImage resolves it through
/proc. On a platform without /proc the image comes back empty,
isOurEngineImage declines, and Uninstall refuses, which the procImage doc
comment describes as the intended behavior there.
The test asserted the Linux outcome on every platform, so on macOS it
failed rather than skipped and left `make test-services` red on a
supported development platform. A red baseline makes a real regression
harder to notice.
Check the precondition rather than the platform: skip only when pidOnPort
resolves a pid but no image. The test starts running again on its own if
the image ever becomes resolvable, without another change here.
Verified on darwin/arm64 with Go 1.26.5:
go test -run TestUninstallTerminatesRunningInstance -count=1 -v ./...
--- SKIP: TestUninstallTerminatesRunningInstance (0.52s)
listener image is unresolvable on darwin, so reclaim declines by design
go test -count=1 ./...
ok nvpair-engine-manager 37.073s
No service binary output changes, so no services/versions.json bump.
Refs: NVIDIA#17
Signed-off-by: Michael Pursifull <mike@arcaven.com>
7 tasks
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.
make test-servicesis red on macOS atmain, which makes a real regression harder to notice on a platform where both shipping engines are supported. This makes the one failing test skip on the precondition it actually needs, so the baseline is green again without changing any behavior.TestUninstallTerminatesRunningInstanceasserts thatUninstallreclaims a managed engine started without an executor process handle. That reclaim needs the listener's executable path, andprocImageresolves it through/proc. Where there is no/procthe image comes back empty,isOurEngineImagedeclines, andUninstallrefuses.procImage's own doc comment describes that as the intended behavior:So the test was asserting the Linux outcome everywhere, and failing rather than skipping where the decline is by design.
The guard checks the precondition rather than the platform:
A
GOOS == "darwin"check would go stale the moment the image becomes resolvable there. This one starts running again on its own, and it skips only on the exact documented condition, a resolved pid with no image, so a genuine lookup failure still reaches the assertion.In scope: one test file, one guard. Out of scope: whether the decline itself should be lifted on macOS, which is #17 and is yours to rule on. This PR is useful either way, and does not presume that answer.
Checks run on darwin/arm64, Go 1.26.5, at
13b6811:I could not run the Linux path, so the claim that the test still executes there rests on
procImagereturning a real path from/proc/<pid>/exe, not on an observed run.No service binary output changes, so no
services/versions.jsonbump.Refs #17