test: isolate unit tests from user state#1883
Open
luozhixiong01 wants to merge 6 commits into
Open
Conversation
github-actions
Bot
added
the
size/S
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLive skills tests now use explicit opt-in gating, isolated temporary environments, shared seeding, and a dedicated Make target. Test packages gain isolated configuration and registry fixtures, while integration tests strengthen filesystem, startup, and strict-mode validation. ChangesTest isolation and integration harnesses
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Makefile
participant prepareLiveSkillsIntegration
participant SkillsCLI
participant UpdateCommand
Makefile->>prepareLiveSkillsIntegration: enable selected live tests
prepareLiveSkillsIntegration->>SkillsCLI: redirect home, config, and npm paths
SkillsCLI-->>UpdateCommand: provide seeded global skills state
UpdateCommand->>SkillsCLI: synchronize skills state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/update/update_test.go`:
- Around line 1696-1701: Update TestUpdateCommand_SkillsSyncColdStart to create
and defer-cancel a 90-second context, then use it for the skills CLI commands.
Before collecting global skills with “skills ls -g”, seed the isolated
environment by running the same “skills add” command for lark-calendar with
global and noninteractive flags, skipping with descriptive errors if listing or
seeding fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@70482b21599dde155f5c2ab35a5007fa0355248c🧩 Skill updatenpx skills add larksuite/cli#fix/isolate-live-skills-tests -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1883 +/- ##
==========================================
+ Coverage 74.66% 74.71% +0.04%
==========================================
Files 877 887 +10
Lines 91731 92673 +942
==========================================
+ Hits 68494 69239 +745
- Misses 17926 18067 +141
- Partials 5311 5367 +56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
luozhixiong01
force-pushed
the
fix/isolate-live-skills-tests
branch
from
ed1060c to
05345c2
Compare
luozhixiong01
changed the title
luozhixiong01
added a commit
that referenced
this pull request
Jul 15, 2026
Address PR #1883 review blockers and additional findings: - Redirect XDG_STATE_HOME in live skills tests: the skills CLI writes its global .skill-lock.json under $XDG_STATE_HOME, escaping the temp home (review P1-1). Also isolate npm_config_cache/prefix/userconfig (both cases) which take precedence over HOME-derived npm defaults. - Seed a tracked registry metadata fixture (calendar/im/task, version 0.0.1) through a new internal/registry/registrytest package so targeted go test passes on a clean checkout with no network, no make fetch_meta, and no user cache (review P1-2). The fixture yields to any real generated catalog; Seed self-checks both directions and neutralizes ambient LARKSUITE_CLI_REMOTE_META/LARKSUITE_CLI_META_TTL overrides. - Apply the same seeding to cmd/auth so its registry-backed completion tests are hermetic instead of reaching for remote metadata (review P1-3). - Fail hard (t.Fatal) on missing live-test preconditions once LARKSUITE_CLI_RUN_LIVE_SKILLS_TESTS=1 is set, so make live-skills-test can no longer skip everything and still report success (review P1-4). - Seed lark-calendar in the cold-start live test so the per-skill assertions run on a non-empty list, and share one preflight helper between both live tests (review P2-5). - Pin the binary autosave isolation contract: parse saved_path, assert the file lands in the temp cwd with the expected content (review P2-6). - Guard TestComputeMinimumScopeSet with ensureFreshRegistry and wait for background refresh goroutines before removing the registry test root. - Isolate internal/cmdutil tests from the developer's real ~/.lark-cli. - Give TestUpdateManual_JSON/Human per-test config dirs so skills-state no longer leaks across tests in the shared package dir.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/registry/registry_test.go (1)
103-110: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not discard the deterministic registry fixture before this assertion.
ensureFreshRegistryresets the seeded registry, points it at an empty directory, and disables remote metadata. On a clean checkout this can makeListFromMetaProjects()empty and silently skip the test. Retain the package-level seed or re-seed beforeInit.Proposed fix
func TestComputeMinimumScopeSet(t *testing.T) { - ensureFreshRegistry(t) minSet := ComputeMinimumScopeSet("user")As per coding guidelines, every behavior change must have a test whose assertions directly pin the changed behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/registry/registry_test.go` around lines 103 - 110, Update TestComputeMinimumScopeSet so ensureFreshRegistry does not discard the deterministic package-level registry fixture before the assertion; retain the seeded registry or re-seed it before initialization. Ensure the test exercises the seeded metadata path and cannot silently skip due to an empty ListFromMetaProjects result, with assertions directly covering the preserved fixture behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/registry/registry_test.go`:
- Around line 103-110: Update TestComputeMinimumScopeSet so ensureFreshRegistry
does not discard the deterministic package-level registry fixture before the
assertion; retain the seeded registry or re-seed it before initialization.
Ensure the test exercises the seeded metadata path and cannot silently skip due
to an empty ListFromMetaProjects result, with assertions directly covering the
preserved fixture behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 303d38fd-2b15-46ae-9942-93363b39ee13
📒 Files selected for processing (13)
cmd/api/api_test.gocmd/auth/testmain_test.gocmd/root_integration_test.gocmd/service/testmain_test.gocmd/startup_brand_test.gocmd/testmain_test.gocmd/update/update_test.gointernal/cmdutil/testmain_test.gointernal/registry/registry_test.gointernal/registry/registrytest/fixture_meta.jsoninternal/registry/registrytest/registrytest.gointernal/registry/registrytest/registrytest_test.gointernal/registry/testmain_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/registry/testmain_test.go
- internal/cmdutil/testmain_test.go
- cmd/auth/testmain_test.go
Address the PR review blockers and follow-up findings in one pass: - Redirect XDG_STATE_HOME in live skills tests: the skills CLI writes its global .skill-lock.json under $XDG_STATE_HOME, escaping the temp home. Also isolate npm_config_cache/prefix/userconfig (both cases), which take precedence over HOME-derived npm defaults. - Seed a tracked, hand-trimmed registry metadata fixture (calendar/im/ task, version 0.0.1, 1.7KB) through a new internal/registry/registrytest package so targeted go test passes on a clean checkout with no network, no make fetch_meta, and no user cache. The fixture yields to any real generated catalog; Seed neutralizes ambient LARKSUITE_CLI_REMOTE_META / LARKSUITE_CLI_META_TTL overrides, refuses config dirs outside the caller's test root, and self-checks both arbitration directions. Its load-bearing content is pinned by a fixture contract test. - Apply the same seeding to cmd, cmd/service and cmd/auth TestMains so registry-backed tests are hermetic instead of reaching for remote metadata; guard the startup-brand re-exec helper with an env + command-argument nonce handshake so a lone inherited environment variable cannot silently disable package isolation. - Fail hard (t.Fatal) on missing live-test preconditions once LARKSUITE_CLI_RUN_LIVE_SKILLS_TESTS=1 is set, so make live-skills-test can no longer skip everything and still report success. Share one preflight helper between both live tests and seed lark-calendar in the cold-start test so its per-skill assertions run on a non-empty list. - Pin the binary autosave isolation contract: parse saved_path, assert the file lands in the temp cwd with the expected content. - Close remaining developer-state leaks: stub the skills index fetch in three update tests that built Updaters directly, isolate internal/cmdutil from the real ~/.lark-cli, give TestUpdateManual_JSON/Human per-test config dirs, guard TestComputeMinimumScopeSet with ensureFreshRegistry, and wait for background refresh goroutines before removing the registry test root. - Decouple the strict-mode bot integration test from registry metadata by using the synthetic fixture catalog.
luozhixiong01
force-pushed
the
fix/isolate-live-skills-tests
branch
from
e0ac9c2 to
075a956
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/testmain_test.go (1)
20-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a helper function to ensure cleanup via
defer.In
TestMain,os.Exitbypasses deferred functions. While explicitos.RemoveAllcalls handle normal exits and setup errors, ifm.Run()panics, the temporary directory will leak.Consider wrapping the setup and execution in a helper function so you can use
deferto guarantee cleanup, which also removes the need for repetitive cleanup calls on error paths.♻️ Proposed refactor
-func TestMain(m *testing.M) { +func TestMain(m *testing.M) { + os.Exit(run(m)) +} + +func run(m *testing.M) int { if isStartupBrandHelper() { // Re-exec helper subprocess (startup_brand_test.go): the parent test // already provides an isolated config dir and disables remote metadata, // and the helper must own the first registry Init to prove the startup // order — do not seed or eagerly initialize here. - os.Exit(m.Run()) + return m.Run() } root, err := os.MkdirTemp("", "lark-cli-cmd-test-*") if err != nil { println("cmd test setup: MkdirTemp failed:", err.Error()) - os.Exit(2) + return 2 } + defer os.RemoveAll(root) + if err := os.Setenv("LARKSUITE_CLI_CONFIG_DIR", root); err != nil { println("cmd test setup: Setenv failed:", err.Error()) - os.RemoveAll(root) - os.Exit(2) + return 2 } if err := registrytest.Seed(root); err != nil { println("cmd test setup: registrytest.Seed failed:", err.Error()) - os.RemoveAll(root) - os.Exit(2) + return 2 } - code := m.Run() - os.RemoveAll(root) - os.Exit(code) + return m.Run() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/testmain_test.go` around lines 20 - 46, Refactor TestMain by moving temporary-directory setup, registrytest.Seed, and m.Run into a helper function that defers os.RemoveAll(root) immediately after creating the directory. Keep TestMain responsible for startup-helper handling and exiting with the helper’s result, and remove repetitive cleanup calls from setup-error paths while preserving their existing exit codes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/testmain_test.go`:
- Around line 20-46: Refactor TestMain by moving temporary-directory setup,
registrytest.Seed, and m.Run into a helper function that defers
os.RemoveAll(root) immediately after creating the directory. Keep TestMain
responsible for startup-helper handling and exiting with the helper’s result,
and remove repetitive cleanup calls from setup-error paths while preserving
their existing exit codes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1b3580a-005a-4e5c-9cd1-35b4ff45be89
📒 Files selected for processing (13)
cmd/api/api_test.gocmd/auth/testmain_test.gocmd/root_integration_test.gocmd/service/testmain_test.gocmd/startup_brand_test.gocmd/testmain_test.gocmd/update/update_test.gointernal/cmdutil/testmain_test.gointernal/registry/registry_test.gointernal/registry/registrytest/fixture_meta.jsoninternal/registry/registrytest/registrytest.gointernal/registry/registrytest/registrytest_test.gointernal/registry/testmain_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/cmdutil/testmain_test.go
- internal/registry/registrytest/fixture_meta.json
- internal/registry/registry_test.go
- cmd/auth/testmain_test.go
- internal/registry/testmain_test.go
- cmd/service/testmain_test.go
- cmd/startup_brand_test.go
- cmd/root_integration_test.go
- cmd/api/api_test.go
- internal/registry/registrytest/registrytest.go
- cmd/update/update_test.go
Seed's success path (cache writes, eager Init, both self-checks) had no coverage from the package's own tests — only the rejection branches did — leaving the patch below the 60% codecov target. Pin it: cache files land under the config dir, LARKSUITE_CLI_REMOTE_META flips to off, and the seeded catalog serves calendar/im/task.
codecov counts atomic-mode partials against the patch, so the happy-path test alone left the diff at 53.8% (target 60%). Exercise the three cache materialization failure branches with same-named files/directories in the way — cross-platform, no permission tricks — which also pins that setup failures surface as errors instead of leaving the registry silently unseeded. Package statement coverage: 72.1%.
4 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
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
Default unit tests contained several side effects that depended on or modified developer state: two opt-in-worthy skills integration tests invoked the real npx skills CLI, ordinary update tests could fall through to npx, multiple packages wrote lark-cli cache/log/event state, registry users started background remote-metadata refreshes, and one API test saved a binary into the repository working directory.
Changes
Validation
The branch is rebased on current origin/main at 1c36744.
Summary by CodeRabbit
make live-skills-test.live-skills-testMakefile target to run the selected live tests with the opt-in flag.