Skip to content

test(versioncheck): isolate the install-probe env and tag the unix-only tests - #2309

Merged
gtrrz-victor merged 2 commits into
mainfrom
soph/versioncheck-test-env-isolation
Sep 7, 2026
Merged

test(versioncheck): isolate the install-probe env and tag the unix-only tests#2309
gtrrz-victor merged 2 commits into
mainfrom
soph/versioncheck-test-env-isolation

Conversation

@Soph

@Soph Soph commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

https://entire.io/gh/entireio/cli/trails/1257

Two test-hygiene items found reviewing #2217, which merged while this was being written — so this targets main rather than stacking. Test-only: no production code changes.

1. The unix table test read the host's mise roots

isolateWindowsInstallEnv clears MISE_INSTALLS_DIR and MISE_DATA_DIR, but it lives in versioncheck_windows_test.go, so the unix table test never got the fix:

MISE_INSTALLS_DIR=/usr/local go test ./cmd/entire/cli/versioncheck/ -run TestUpdateCommandForCurrentBinary_Unix
--- FAIL: .../unknown_path_stable_falls_back_to_stable_curl_command
    UpdateCommandForCurrentBinary() = "mise upgrade entire", want "curl -fsSL https://entire.io/install.sh | bash"
--- FAIL: .../unknown_path_nightly_falls_back_to_nightly_curl_command

MISE_INSTALLS_DIR is the only probe variable whose value becomes a root verbatim — miseRoots returns it as-is, while MISE_DATA_DIR gets installs appended and scoopRoots appends apps to all four of its inputs. That mandatory extra segment is what stops the others prefixing an arbitrary path, and it is also why no value of SCOOP could reproduce the Windows half of the finding this fell out of: SCOOP=C:\tools yields the root c:/tools/apps/, which does not prefix c:/tools/cli/entire.exe.

So the exposed rows are exactly the two whose expectation is the fallback — the ones asserting that no probe matched — and one variable reaches them. The brew rows are immune by probe order, and the executable error row returns before the probe loop.

Fix: the two Setenv calls move into an untagged isolateMiseInstallEnv that both platforms share, with isolateWindowsInstallEnv wrapping it and adding Scoop's, so the pair cannot drift again.

Env Before After
MISE_INSTALLS_DIR=/usr/local 2 rows FAIL pass
MISE_DATA_DIR=/usr pass (needs an installs/ segment) pass
MISE_INSTALLS_DIR=usr/local pass (normalizeInstallRoot rejects relative) pass

CI is unaffected either way — no runner sets these. It is the developer-box run that lies, with a message pointing at the code under test and no hint that the environment caused it.

2. Three CheckAndNotify tests were untagged but unix-only

TestCheckAndNotify_BrewSkipUntilNextVersionCachesLatest, ..._MiseSkipUntilNextVersionCachesLatest and ..._InstallerFailureKeepsCacheFresh all assert what happens after maybeAutoUpdate reaches the prompt — that chooseUpdate's answer lands in the cache as SkippedVersion, or that the Try again later running: hint prints. On Windows maybeAutoUpdate returns autoUpdateActionSkip before any of that, so all three fail on a Windows host. Confirmed by building the package with installerAutoRuns flipped to false.

CI cannot see it: test-windows selects by name (-run '(Windows|MSYS)') and none of these match. The previous escape hatch was the goos variable seam and its pinNonWindowsGOOS helper, which #2217 removed — correctly — leaving these three as the callers it did not follow up. A build tag is now the only way to express what they need.

They move to versioncheck_unix_test.go alongside the existing unix-only CheckAndNotify coverage, and brewUpgradeCmd goes with them since brew is a unix-only probe here. brewCaskPath stays untagged: TestCheckAndNotify_SkipsVersionMarkedSkipped still uses it and is genuinely platform-agnostic, because CheckAndNotify returns on the skipped version before any installer is consulted.

Verified with go list that the Windows test build now contains only autoupdate_windows_test.go, fixture_test.go, versioncheck_test.go and versioncheck_windows_test.go.

Known gap, deliberately not closed here

Nothing asserts the Windows side of the same contract — that CheckAndNotify does not cache a skip there, which is the deliberate "the nudge returns every 24h until they update" behaviour maybeAutoUpdate documents. That wants a new windows-tagged test rather than a moved one, so it is out of scope.

Verification

mise run lint 0 issues, mise run test:ci fully green, GOOS=windows go vet ./... clean.

🤖 Generated with Claude Code


Note

Low Risk
Only test layout and environment isolation; no runtime version-check or auto-update logic changes.

Overview
Test-only hardening for versioncheck: no production behavior changes.

Adds shared isolateMiseInstallEnv (clears MISE_INSTALLS_DIR / MISE_DATA_DIR) so host mise roots cannot make “unknown path → curl install” cases look like mise upgrade. The Unix UpdateCommandForCurrentBinary table calls it in each subtest; isolateWindowsInstallEnv now delegates to the same helper instead of duplicating those Setenv calls.

Moves three CheckAndNotify tests that depend on interactive auto-update (skip-until-next-version caching and installer-failure retry hint) into versioncheck_unix_test.go under the unix build tag, because on Windows installerAutoRuns is false and maybeAutoUpdate bails out before those paths run. brewUpgradeCmd moves with them; shared versioncheck_test.go drops the duplicated tests and unused imports.

Reviewed by Cursor Bugbot for commit dfef5ab. Configure here.

Soph and others added 2 commits September 7, 2026 14:28
isolateWindowsInstallEnv cleared MISE_INSTALLS_DIR and MISE_DATA_DIR, but it
is windows-tagged, so TestUpdateCommandForCurrentBinary_Unix kept reading the
host's:

    MISE_INSTALLS_DIR=/usr/local go test ./cmd/entire/cli/versioncheck/ \
        -run TestUpdateCommandForCurrentBinary_Unix
    --- FAIL: .../unknown_path_stable_falls_back_to_stable_curl_command
        UpdateCommandForCurrentBinary() = "mise upgrade entire",
        want "curl -fsSL https://entire.io/install.sh | bash"

MISE_INSTALLS_DIR is the only probe variable whose value becomes a root
verbatim: miseRoots returns it as-is, while MISE_DATA_DIR gets "installs"
appended and scoopRoots appends "apps" to all four of its inputs. That
mandatory extra segment is what stops the others prefixing an arbitrary path,
which is also why no value of SCOOP could reproduce the Windows half of the
finding this fell out of. So the exposed rows are the two asserting that NO
probe matched, and one variable reaches them.

Move the two Setenv calls into an untagged isolateMiseInstallEnv so both
platforms share one helper — the Windows one wraps it and adds Scoop's — rather
than letting the pair drift again. Nothing here affects CI, where none of these
variables are set; it is the developer-box run that lies.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M1XX0CE04KSHZ22BJWPJ8S9B
Three tests in the untagged versioncheck_test.go only hold where
installerAutoRuns is true, because they assert what happens *after*
maybeAutoUpdate reaches the prompt:

  - BrewSkipUntilNextVersionCachesLatest and MiseSkipUntilNextVersionCachesLatest
    expect chooseUpdate's answer to land in the cache as SkippedVersion
  - InstallerFailureKeepsCacheFresh expects the "Try again later running:" hint

On Windows maybeAutoUpdate returns autoUpdateActionSkip before any of that, so
lastCmdStr stays empty, SkippedVersion is never written, and the hint never
prints. All three fail on a Windows host. Confirmed by building the package
with installerAutoRuns flipped to false.

CI cannot see it: the test-windows job selects by name (-run '(Windows|MSYS)'),
and none of these match. Until now the escape hatch was the goos variable seam
and its pinNonWindowsGOOS helper, which the per-OS split removed — correctly,
but these three were the callers it left behind, so a build tag is now the only
way to express what they need.

Move them to versioncheck_unix_test.go, alongside the existing unix-only
CheckAndNotify coverage, and take brewUpgradeCmd with them since brew is a
unix-only probe here. brewCaskPath stays untagged: SkipsVersionMarkedSkipped
still uses it and is genuinely platform-agnostic, since CheckAndNotify returns
on the skipped version before any installer is consulted.

Verified with go list that the Windows test build now contains only
autoupdate_windows_test.go, fixture_test.go, versioncheck_test.go and
versioncheck_windows_test.go, and that GOOS=windows go vet is clean.

The gap this leaves is worth naming: nothing asserts the Windows side of the
same contract — that CheckAndNotify does NOT cache a skip there, which is the
deliberate "the nudge returns every 24h" behaviour maybeAutoUpdate documents.
That wants a windows-tagged test, not a moved one, so it is out of scope here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M1XX536ZZQX16VHF8AM0NA2Q
Copilot AI lite review requested due to automatic review settings September 7, 2026 12:29
@Soph
Soph requested a review from a team as a code owner September 7, 2026 12:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

Changes are test-only, scoped, and align build tags/env isolation with the intended platform-specific behaviors.

Pull request overview

This PR improves versioncheck test hygiene by (1) isolating mise-related environment variables across platforms so host mise roots don’t affect update-command detection tests, and (2) correctly scoping unix-only auto-update tests behind the unix build tag to avoid Windows-only behavior differences.

Changes:

  • Adds a shared isolateMiseInstallEnv test helper to clear MISE_INSTALLS_DIR / MISE_DATA_DIR, and uses it from both unix and Windows tests.
  • Updates the unix UpdateCommandForCurrentBinary table test to call isolateMiseInstallEnv per subtest, preventing host environment leakage.
  • Moves three CheckAndNotify tests that rely on unix auto-update behavior into versioncheck_unix_test.go (//go:build unix) and removes them from the untagged test file.
File summaries
File Description
cmd/entire/cli/versioncheck/versioncheck_windows_test.go Reuses shared mise env isolation from Windows-specific install env helper.
cmd/entire/cli/versioncheck/versioncheck_unix_test.go Adds per-subtest mise env isolation and relocates unix-only CheckAndNotify test coverage under the unix build tag.
cmd/entire/cli/versioncheck/versioncheck_test.go Removes unix-only tests/constants from the untagged cross-platform test file.
cmd/entire/cli/versioncheck/fixture_test.go Introduces shared isolateMiseInstallEnv helper for cross-platform test env isolation.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@gtrrz-victor
gtrrz-victor merged commit 60d5fe4 into main Sep 7, 2026
15 checks passed
@gtrrz-victor
gtrrz-victor deleted the soph/versioncheck-test-env-isolation branch September 7, 2026 12:50
MuskanPaliwal pushed a commit to MuskanPaliwal/cli that referenced this pull request Sep 7, 2026
A finding on entireio#2309 claimed the three CheckAndNotify tests moved there fail when
a developer has MISE_INSTALLS_DIR set wide enough to cover brewCaskPath (it
suggested /opt). The premise is right and the conclusion is not: both probes
match, but UpdateCommandForCurrentBinary returns the FIRST match and
installProbes orders brewProbe ahead of miseProbe, so brew wins.

    miseProbe.matches("/opt/homebrew/Caskroom/entire/1.0.0/entire") = true
    brewProbe.matches("/opt/homebrew/Caskroom/entire/1.0.0/entire") = true
    UpdateCommandForCurrentBinary() = "brew upgrade --yes entire"

Confirmed against every MISE_INSTALLS_DIR worth trying, /opt and / included,
plus MISE_DATA_DIR. The comparison to 73a3d39 does not hold either: that fixed
the two table rows whose expectation is the *fallback*, where by definition no
probe matched, so precedence could not rescue them. Here it does.

What the finding did surface is that precedence is load-bearing and unpinned.
It is a product invariant, not just test scaffolding: someone can have entire
installed by brew and mise relocated under a broad root, and that user has to
be told to run brew. Reordering installProbes would silently change the advice,
and would break those tests only on the developer boxes that set the variable.

So pin it, asserting the precondition as well as the outcome — without a
contending probe the test would keep passing while testing nothing. Verified by
mutation: flipping installProbes fails on the outcome, and stopping mise
matching fails on the precondition.

Also fold isolateMiseInstallEnv into newAutoUpdateFixture, which is already
where a test's environment is made hermetic (HOME, kill switch, TTY), and into
the one test that wires the same seams inline instead of using the fixture.
That is decoupling rather than a fix — nothing was failing — so that the tests
no longer depend on precedence at all, and the invariant is asserted in one
place that says why.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 01M1Y02AWEH7JCQ00J806YVMNC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants