Skip to content

test(drive): drop redundant CONFIG_DIR isolation in inspect Execute tests#1121

Merged
fangshuyu-768 merged 1 commit into
larksuite:mainfrom
kyalpha313:chore/drop-redundant-config-dir-inspect-test
May 28, 2026
Merged

test(drive): drop redundant CONFIG_DIR isolation in inspect Execute tests#1121
fangshuyu-768 merged 1 commit into
larksuite:mainfrom
kyalpha313:chore/drop-redundant-config-dir-inspect-test

Conversation

@kyalpha313
Copy link
Copy Markdown
Contributor

@kyalpha313 kyalpha313 commented May 27, 2026

Summary

drive_inspect_test.go 的六个 TestDriveInspectExecute_* 测试都调用了 t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()),但它们通过 cmdutil.TestFactory(t, cfg) 构建 CLI。TestFactory 提供的是内存 config 闭包(Config: func() (*core.CliConfig, error) { return config, nil }),不读文件系统,所以这个环境变量对这些测试毫无作用。

Changes

  • 删除 drive_inspect_test.go 中 6 处冗余的 t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())

依据

仓库已有 learning(来自 #343):

Only set LARKSUITE_CLI_CONFIG_DIR when the test exercises the real NewDefault() factory path that reads from the filesystem. Shortcut tests that build the CLI via cmdutil.TestFactory(t, config) should not set this env var because TestFactory provides an in-memory config closure and does not access the filesystem.

已确认这 6 个测试全部使用 cmdutil.TestFactorygrep -c NewDefault 为 0,因此这些调用是 dead code。

(这是在 #1114 的 review 讨论中发现的既有不一致,单独拆成一个 PR。)

Test Plan

  • go test -race -count=1 ./shortcuts/drive/ -run TestDriveInspect 通过
  • gofmt -l 无输出
  • go vet ./shortcuts/drive/ 通过
  • 无行为变更(纯删除无效的环境变量设置)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Simplified test environment setup across multiple test cases.

Note: This release contains internal testing optimizations with no user-visible changes or new functionality.

Review Change Stack

…ests

The six TestDriveInspectExecute_* tests set
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) but build the CLI via
cmdutil.TestFactory(t, cfg), which provides an in-memory config closure
(func() (*core.CliConfig, error) { return config, nil }) and never reads the
filesystem. Per the repo learning from PR larksuite#343, this env var should only be
set for tests exercising the real NewDefault() factory path. None of these
tests use NewDefault(), so the calls are dead and removed.

No behavior change; all TestDriveInspect* tests still pass.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c6071ebc-ad4e-4654-a116-38dc632d8aaa

📥 Commits

Reviewing files that changed from the base of the PR and between 367cfc9 and e755e20.

📒 Files selected for processing (1)
  • shortcuts/drive/drive_inspect_test.go
💤 Files with no reviewable changes (1)
  • shortcuts/drive/drive_inspect_test.go

📝 Walkthrough

Walkthrough

Six test functions in shortcuts/drive/drive_inspect_test.go have identical per-test environment variable initialization removed. The t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) call is deleted from DocxURL, WikiURL, WikiGetNodeIncompleteData, BareTokenWithType, BatchQueryError, and PrettyFormat tests without changing test signatures or assertions.

Changes

Test Environment Setup Cleanup

Layer / File(s) Summary
Remove per-test environment setup from DriveInspect tests
shortcuts/drive/drive_inspect_test.go
Six test functions remove identical t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) calls, likely consolidating setup to a shared test fixture or removing redundant initialization.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested reviewers

  • wittam-01

Poem

🐰 Six tests shed their setup calls with care,
Temp directories vanish into air,
Cleanup hops along without a care,
Simpler tests now live so fair! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: removing redundant CONFIG_DIR isolation setup from inspect Execute tests in the drive package.
Description check ✅ Passed The description covers all required sections: summary (explaining the redundancy), changes (specific deletions), test plan (with verification steps), and related issues reference. The description is comprehensive and well-structured.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 unit tests (beta)
  • Create PR with unit tests

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 domain/ccm PR touches the ccm domain size/S Low-risk docs, CI, test, or chore only changes labels May 27, 2026
@github-actions
Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

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

🧩 Skill update

npx skills add kyalpha313/lark-cli#chore/drop-redundant-config-dir-inspect-test -y -g

@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.10%. Comparing base (367cfc9) to head (e755e20).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1121   +/-   ##
=======================================
  Coverage   68.10%   68.10%           
=======================================
  Files         613      613           
  Lines       56396    56396           
=======================================
  Hits        38409    38409           
  Misses      14809    14809           
  Partials     3178     3178           

☔ 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.

@fangshuyu-768 fangshuyu-768 merged commit 3cd84fc into larksuite:main May 28, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/ccm PR touches the ccm domain 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