Skip to content

test(update): isolate stamp writes from real ~/.lark-cli/skills.stamp#858

Merged
liangshuo-1 merged 1 commit into
mainfrom
fix/update-test-stamp-isolation
May 13, 2026
Merged

test(update): isolate stamp writes from real ~/.lark-cli/skills.stamp#858
liangshuo-1 merged 1 commit into
mainfrom
fix/update-test-stamp-isolation

Conversation

@liangshuo-1

@liangshuo-1 liangshuo-1 commented May 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Five tests in cmd/update/update_test.go mocked SkillsUpdateOverride to return success and let runSkillsAndStamp call WriteStamp, but did not isolate LARKSUITE_CLI_CONFIG_DIR. Each run clobbered the real ~/.lark-cli/skills.stamp with the mocked version (`"2.0.0"` or `"1.0.0"`), causing skillscheck.Init to fire a misleading drift notice (`lark-cli skills X out of sync with binary Y`) on every subsequent invocation.
  • Patch: add `t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())` at the top of:
    • `TestUpdateNpm_JSON` (writes `"2.0.0"`)
    • `TestUpdateNpm_Human` (writes `"2.0.0"`)
    • `TestUpdateForce_JSON` (writes `"1.0.0"`)
    • `TestUpdateDevVersion_JSON` (writes `"1.0.0"`)
    • `TestUpdateWindows_NpmSuccess_JSON` (writes `"2.0.0"`)
  • Scope intentionally limited to tests that mock `SkillsUpdateOverride` to success. Tests that invoke real `npx` are pre-existing slow/network behavior, out of scope here.

Test plan

  • `go test ./cmd/update/ -run '^(TestUpdateNpm_JSON|TestUpdateNpm_Human|TestUpdateForce_JSON|TestUpdateDevVersion_JSON|TestUpdateWindows_NpmSuccess_JSON)$' -v` — all 5 pass in <10ms each
  • Verified `~/.lark-cli/skills.stamp` not mutated after run
  • `go vet ./cmd/update/...` clean

Summary by CodeRabbit

  • Tests
    • Improved test isolation by configuring temporary directories for configuration storage, preventing tests from affecting real user configuration files.

Review Change Stack

Five tests in cmd/update mocked SkillsUpdateOverride to return success
and let runSkillsAndStamp call WriteStamp, but did not isolate
LARKSUITE_CLI_CONFIG_DIR. Each run clobbered the real
~/.lark-cli/skills.stamp with the mock version ("2.0.0" or "1.0.0"),
causing skillscheck to fire a misleading drift notice on every
subsequent lark-cli invocation.

Add t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the top of:
  - TestUpdateNpm_JSON
  - TestUpdateNpm_Human
  - TestUpdateForce_JSON
  - TestUpdateDevVersion_JSON
  - TestUpdateWindows_NpmSuccess_JSON

Scope is limited to tests that mock SkillsUpdateOverride to success;
tests that invoke real npx are pre-existing and out of scope here.

Change-Id: I7a78a6c70f276b51333253acc115e0109c01a851
@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 35326100-40b4-4caf-83a9-55736757a1a3

📥 Commits

Reviewing files that changed from the base of the PR and between ce0b68d and 7641a3b.

📒 Files selected for processing (1)
  • cmd/update/update_test.go

📝 Walkthrough

Walkthrough

Five test cases in the update module are updated to isolate filesystem side effects by setting LARKSUITE_CLI_CONFIG_DIR to temporary directories. This prevents tests from writing to the user's real ~/.lark-cli/skills.stamp file when executing mocked update flows.

Changes

Test Filesystem Isolation

Layer / File(s) Summary
Isolate config directory in update test cases
cmd/update/update_test.go
LARKSUITE_CLI_CONFIG_DIR is set to t.TempDir() in five test cases (TestUpdateNpm_JSON, TestUpdateNpm_Human, TestUpdateForce_JSON, TestUpdateDevVersion_JSON, TestUpdateWindows_NpmSuccess_JSON) with explanatory comments that the isolation prevents overwriting the real ~/.lark-cli/skills.stamp file during test execution with mocked fetchLatest values.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • larksuite/cli#723: Both PRs modify cmd/update/update_test.go to isolate LARKSUITE_CLI_CONFIG_DIR in update tests to prevent writing to the user's real ~/.lark-cli/skills.stamp when using mocked fetchLatest values.

Suggested labels

bug

Suggested reviewers

  • evandance

Poem

🐰 A rabbit hops through test files with glee,
Setting temp dirs where config should be,
No stamps on the system, just toys in a box,
Tests run in isolation, no pesky side-locks! 🎯

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: isolating test filesystem operations to prevent contamination of the real user config directory.
Description check ✅ Passed The description comprehensively covers all template sections: clear summary of the problem, specific list of affected tests with patch details, and detailed test plan with verification results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/update-test-stamp-isolation

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the size/S Low-risk docs, CI, test, or chore only changes label May 13, 2026
@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.85%. Comparing base (ce0b68d) to head (7641a3b).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #858   +/-   ##
=======================================
  Coverage   65.85%   65.85%           
=======================================
  Files         517      517           
  Lines       48761    48761           
=======================================
  Hits        32113    32113           
  Misses      13885    13885           
  Partials     2763     2763           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@7641a3ba51d7adda20fd00001169e2ea9a399f17

🧩 Skill update

npx skills add larksuite/cli#fix/update-test-stamp-isolation -y -g

@liangshuo-1
liangshuo-1 merged commit 25454f4 into main May 13, 2026
21 checks passed
@liangshuo-1
liangshuo-1 deleted the fix/update-test-stamp-isolation branch May 13, 2026 05:52
tuxedomm pushed a commit to zhumiaoxin/cli that referenced this pull request Jun 6, 2026
…larksuite#858)

Five tests in cmd/update mocked SkillsUpdateOverride to return success
and let runSkillsAndStamp call WriteStamp, but did not isolate
LARKSUITE_CLI_CONFIG_DIR. Each run clobbered the real
~/.lark-cli/skills.stamp with the mock version ("2.0.0" or "1.0.0"),
causing skillscheck to fire a misleading drift notice on every
subsequent lark-cli invocation.

Add t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the top of:
  - TestUpdateNpm_JSON
  - TestUpdateNpm_Human
  - TestUpdateForce_JSON
  - TestUpdateDevVersion_JSON
  - TestUpdateWindows_NpmSuccess_JSON

Scope is limited to tests that mock SkillsUpdateOverride to success;
tests that invoke real npx are pre-existing and out of scope here.

Change-Id: I7a78a6c70f276b51333253acc115e0109c01a851
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Low-risk docs, CI, test, or chore only changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants