feat(test): Add a CLI E2E testing framework for lark-cli, task domain testcase and ci action#236
Merged
feat(test): Add a CLI E2E testing framework for lark-cli, task domain testcase and ci action#236
Conversation
github-actions
bot
added
the
size/M
📝 WalkthroughWalkthroughAdds a CLI end-to-end test suite and CI integration: new GitHub Actions workflow for CLI E2E tests, a clie2e Go test harness and helpers, multiple scenario-based E2E tests under Changes
Sequence DiagramsequenceDiagram
participant Test as Test Function
participant RunCmd as clie2e.RunCmd
participant Resolve as ResolveBinaryPath
participant Build as BuildArgs
participant Exec as exec.CommandContext
participant Binary as lark-cli
participant Parse as JSON Parser
Test->>RunCmd: RunCmd(ctx, Request)
RunCmd->>Resolve: Resolve binary path (explicit → env → project-relative → PATH)
Resolve-->>RunCmd: Binary path
RunCmd->>Build: BuildArgs(req) (append --as, --format, --params, --data)
Build-->>RunCmd: Final CLI args
RunCmd->>Exec: Execute command, capture stdout/stderr
Exec->>Binary: Invoke lark-cli
Binary-->>Exec: Exit code + output
Exec-->>RunCmd: Captured stdout/stderr
RunCmd->>Parse: Parse stdout/stderr as JSON (if needed)
Parse-->>RunCmd: Parsed objects
RunCmd-->>Test: Result{ExitCode, Stdout, Stderr, RunErr}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@88d1b1022bee10fa8b89b52db6b8c38dcf41c5c8🧩 Skill updatenpx skills add larksuite/cli#feat/e2e_test -y -g |
Greptile SummaryThis PR introduces a full CLI E2E test framework ( Confidence Score: 5/5Safe to merge; all previously flagged blocking issues are resolved and remaining findings are minor robustness suggestions. Both P1 concerns from earlier rounds are fixed. The two remaining findings are P2: a missing file-existence guard in the CI summary step and a confusing type-mismatch path in AssertStdoutStatus. Neither affects correctness of the E2E tests themselves. .github/workflows/cli-e2e.yml (XML summary guard) and tests/cli_e2e/core.go (AssertStdoutStatus type clarity) — both are minor quality improvements. Important Files Changed
Sequence DiagramsequenceDiagram
participant CI as GitHub Actions
participant Build as make build
participant Config as lark-cli config init
participant GoTest as go test (gotestsum)
participant Harness as clie2e.RunCmd
participant CLI as lark-cli binary
participant API as Lark API
CI->>Build: make build → lark-cli binary
CI->>Config: config init --app-id / --app-secret-stdin
CI->>GoTest: run tests/cli_e2e/task/...
GoTest->>Harness: clie2e.RunCmd(ctx, Request{Args, Params, Data})
Harness->>Harness: ResolveBinaryPath (env → project root → PATH)
Harness->>CLI: exec lark-cli config default-as bot (once)
Harness->>CLI: exec lark-cli <args> --params <json> --data <json>
CLI->>API: HTTP request (bot credentials)
API-->>CLI: JSON response
CLI-->>Harness: stdout JSON + exit code
Harness-->>GoTest: Result{Stdout, ExitCode, ...}
GoTest->>GoTest: AssertExitCode / AssertStdoutStatus / gjson assertions
GoTest-->>CI: JUnit XML report
CI->>CI: Python summary → GITHUB_STEP_SUMMARY
Reviews (4): Last reviewed commit: "feat(test): add cli e2e config and fix t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/cli_e2e/core_test.go (2)
326-328: Consider adding a comment explaining the sync.Once reset pattern.Resetting
sync.Onceby direct assignment works but is unconventional. A brief comment would help future maintainers understand this is intentional test infrastructure.Proposed documentation
+// resetDefaultAsInitForTest resets the package-level sync.Once to allow +// tests to verify the "initialize once" behavior independently. +// This is only valid in test code within the same package. func resetDefaultAsInitForTest() { defaultAsInitOnce = sync.Once{} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/core_test.go` around lines 326 - 328, The test helper resetDefaultAsInitForTest currently resets the package-level sync.Once by direct assignment to defaultAsInitOnce = sync.Once{} without explanation; add a concise comment above the resetDefaultAsInitForTest function (or inside it) explaining that assigning a zero-value to a sync.Once is an intentional pattern used only for tests to allow re-running initialization, and note any caveats (e.g., only safe in tests because it breaks Once semantics for real concurrency). This clarifies intent for future maintainers and references the reset of defaultAsInitOnce and the use of sync.Once.
41-58: Good test for project root resolution, but consider parallel test safety.The test changes the global working directory which could interfere with other tests if run in parallel. Consider adding
t.Parallel()constraints or documenting that this test cannot run in parallel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/core_test.go` around lines 41 - 58, This test mutates the global working directory (os.Chdir) which can race with parallel tests; either mark the test safe for parallel execution by adding t.Parallel() at the top of the test and ensuring the test uses an isolated temp directory for all file operations, or explicitly document/guard that it must not run in parallel (add a comment and avoid calling t.Parallel in this file). Locate the test that calls ResolveBinaryPath(Request{}), touches projectRootMarkerDir, uses cliBinaryName, sets EnvBinaryPath, and performs os.Getwd/os.Chdir; then either add t.Parallel() as the first line of that t.Run block and confirm all file ops use tmpDir, or add a clear comment stating the test cannot run in parallel (and do not call t.Parallel).tests/cli_e2e/core.go (1)
237-251: Type coercion in AssertStdoutStatus may produce confusing failures.When
expectedistrue(bool) but the JSON contains"code": 0, the assertion will fail because it checksokfirst (which doesn't exist) then checkscodeagainst a bool. Similarly, passing0(int) when the response has"ok": truewill fail.The current callers use this correctly (bool for
okresponses, int forcoderesponses), but the API could be clearer:Consider separate assertion methods for clarity
// AssertStdoutOK asserts stdout JSON has {"ok": <expected>}. func (r *Result) AssertStdoutOK(t *testing.T, expected bool) { t.Helper() okResult := gjson.Get(r.Stdout, "ok") if !okResult.Exists() { assert.Fail(t, "stdout missing 'ok' field", "stdout:\n%s", r.Stdout) return } assert.Equal(t, expected, okResult.Bool(), "stdout:\n%s", r.Stdout) } // AssertStdoutCode asserts stdout JSON has {"code": <expected>}. func (r *Result) AssertStdoutCode(t *testing.T, expected int) { t.Helper() codeResult := gjson.Get(r.Stdout, "code") if !codeResult.Exists() { assert.Fail(t, "stdout missing 'code' field", "stdout:\n%s", r.Stdout) return } assert.Equal(t, expected, int(codeResult.Int()), "stdout:\n%s", r.Stdout) }This would make test intent explicit:
result.AssertStdoutOK(t, true)vsresult.AssertStdoutCode(t, 0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/core.go` around lines 237 - 251, The current AssertStdoutStatus method on Result ambiguously coerces expected into either bool or int depending on which key exists and can cause confusing failures; replace it with two explicit methods: AssertStdoutOK(t *testing.T, expected bool) which checks that gjson.Get(r.Stdout, "ok") exists then compares okResult.Bool() to expected, and AssertStdoutCode(t *testing.T, expected int) which checks that gjson.Get(r.Stdout, "code") exists then compares int(codeResult.Int()) to expected; remove or deprecate AssertStdoutStatus to force callers to use AssertStdoutOK or AssertStdoutCode and update any call sites accordingly..github/workflows/cli-e2e.yml (1)
62-71: Consider quoting the packages variable to prevent globbing issues.While word splitting is intentional for passing multiple packages to the test command, using an array or explicit handling would be safer.
Proposed improvement for robustness
- name: Run CLI E2E tests env: LARK_CLI_BIN: ${{ github.workspace }}/lark-cli run: | - packages=$(go list ./tests/cli_e2e/... | grep -v '^github.com/larksuite/cli/tests/cli_e2e$' | grep -v '/demo$') + packages=$(go list ./tests/cli_e2e/... | grep -v '^github.com/larksuite/cli/tests/cli_e2e$' | grep -v '/demo$' | tr '\n' ' ') if [ -z "$packages" ]; then echo "No CLI E2E packages to test after exclusions." exit 1 fi - go run gotest.tools/gotestsum@v1.12.3 --format testname --junitfile cli-e2e-report.xml -- -count=1 -v $packages + # shellcheck disable=SC2086 - intentional word splitting + go run gotest.tools/gotestsum@v1.12.3 --format testname --junitfile cli-e2e-report.xml -- -count=1 -v $packages🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/cli-e2e.yml around lines 62 - 71, The packages word-splitting is intentional but currently risks filename globbing; convert the whitespace-separated packages string into a bash array and pass it safely to the go test command. After computing packages, use mapfile -t (or readarray -t) to populate an array (e.g., pkg_array) from "$packages" and then invoke the test runner with "${pkg_array[@]}" in place of $packages so each package is a separate arg and globbing is avoided; update the Run step accordingly referencing the packages variable and pkg_array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/cli-e2e.yml:
- Around line 73-80: The step named "Publish CLI E2E test report" uses an
invalid condition syntax; replace the plain YAML condition `(!cancelled())` with
a GitHub Actions expression wrapper by changing the step's if to use `${{
!cancelled() }}` (or `${{ !cancelled }}`) so the runner evaluates the expression
correctly; update the if field in that step to the wrapped expression.
In `@tests/cli_e2e/core.go`:
- Around line 64-66: The call in defaultAsInitOnce.Do currently ignores any
error returned by setDefaultAs; update the initialization to capture the
returned error from setDefaultAs(binaryPath, defaultIdentity) and handle it (at
minimum log it via the test logger or call t.Fatalf/t.Logf as appropriate) so
CI-visible failures surface the root cause; modify the closure used with
defaultAsInitOnce.Do to check the error value and record or fail the test rather
than discarding it.
---
Nitpick comments:
In @.github/workflows/cli-e2e.yml:
- Around line 62-71: The packages word-splitting is intentional but currently
risks filename globbing; convert the whitespace-separated packages string into a
bash array and pass it safely to the go test command. After computing packages,
use mapfile -t (or readarray -t) to populate an array (e.g., pkg_array) from
"$packages" and then invoke the test runner with "${pkg_array[@]}" in place of
$packages so each package is a separate arg and globbing is avoided; update the
Run step accordingly referencing the packages variable and pkg_array.
In `@tests/cli_e2e/core_test.go`:
- Around line 326-328: The test helper resetDefaultAsInitForTest currently
resets the package-level sync.Once by direct assignment to defaultAsInitOnce =
sync.Once{} without explanation; add a concise comment above the
resetDefaultAsInitForTest function (or inside it) explaining that assigning a
zero-value to a sync.Once is an intentional pattern used only for tests to allow
re-running initialization, and note any caveats (e.g., only safe in tests
because it breaks Once semantics for real concurrency). This clarifies intent
for future maintainers and references the reset of defaultAsInitOnce and the use
of sync.Once.
- Around line 41-58: This test mutates the global working directory (os.Chdir)
which can race with parallel tests; either mark the test safe for parallel
execution by adding t.Parallel() at the top of the test and ensuring the test
uses an isolated temp directory for all file operations, or explicitly
document/guard that it must not run in parallel (add a comment and avoid calling
t.Parallel in this file). Locate the test that calls
ResolveBinaryPath(Request{}), touches projectRootMarkerDir, uses cliBinaryName,
sets EnvBinaryPath, and performs os.Getwd/os.Chdir; then either add t.Parallel()
as the first line of that t.Run block and confirm all file ops use tmpDir, or
add a clear comment stating the test cannot run in parallel (and do not call
t.Parallel).
In `@tests/cli_e2e/core.go`:
- Around line 237-251: The current AssertStdoutStatus method on Result
ambiguously coerces expected into either bool or int depending on which key
exists and can cause confusing failures; replace it with two explicit methods:
AssertStdoutOK(t *testing.T, expected bool) which checks that
gjson.Get(r.Stdout, "ok") exists then compares okResult.Bool() to expected, and
AssertStdoutCode(t *testing.T, expected int) which checks that
gjson.Get(r.Stdout, "code") exists then compares int(codeResult.Int()) to
expected; remove or deprecate AssertStdoutStatus to force callers to use
AssertStdoutOK or AssertStdoutCode and update any call sites accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c809658c-c2ba-4238-8e8b-ea8d9173601d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.github/workflows/cli-e2e.ymlgo.modtests/cli_e2e/README.mdtests/cli_e2e/cli-e2e-testcase-writer/SKILL.mdtests/cli_e2e/core.gotests/cli_e2e/core_test.gotests/cli_e2e/demo/task_lifecycle_test.gotests/cli_e2e/task/helpers_test.gotests/cli_e2e/task/task_comment_workflow_test.gotests/cli_e2e/task/task_reminder_workflow_test.gotests/cli_e2e/task/task_status_workflow_test.gotests/cli_e2e/task/tasklist_add_task_workflow_test.gotests/cli_e2e/task/tasklist_workflow_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/cli_e2e/core.go (1)
67-69:⚠️ Potential issue | 🟡 MinorSilent
setDefaultAsfailure is still being discarded.Line 68 still ignores the init error, which makes root-cause diagnosis in CI harder when identity setup is the real failure.
Suggested fix
+var defaultAsInitErr error + defaultAsInitOnce.Do(func() { - _ = setDefaultAs(binaryPath, defaultIdentity) + defaultAsInitErr = setDefaultAs(binaryPath, defaultIdentity) + if defaultAsInitErr != nil { + fmt.Fprintf(os.Stderr, "warning: failed to set default-as: %v\n", defaultAsInitErr) + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/core.go` around lines 67 - 69, The call inside defaultAsInitOnce.Do currently discards the error from setDefaultAs (called with binaryPath and defaultIdentity); change this to capture the returned error, handle it by logging or failing the test setup (e.g., call t.Fatalf or return the error) so CI shows the root cause—update the anonymous func passed to defaultAsInitOnce.Do to assign err := setDefaultAs(binaryPath, defaultIdentity) and then handle err appropriately rather than ignoring it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cli_e2e/core.go`:
- Around line 58-69: The RunCmd flow (ResolveBinaryPath, defaultAsInitOnce,
setDefaultAs) is flagged by GO-2026-4602; fix it by upgrading the Go toolchain
used to build and lint the repo to a patched release (Go >= 1.25.8 or >=
1.26.1). Update your CI configuration (GitHub Actions / Makefile /
Dockerfile/etc.) to use Go 1.25.8 or 1.26.1+, update the go directive in go.mod
if present, run go mod tidy and re-run the linter/tests to confirm the
vulnerability is no longer reported for RunCmd, ResolveBinaryPath, and related
code paths.
- Around line 189-194: The setDefaultAs function can hang because it uses
exec.Command without a context; change its signature to accept ctx
context.Context (i.e., setDefaultAs(ctx context.Context, binaryPath string,
identity string) error), replace exec.Command(...) with exec.CommandContext(ctx,
binaryPath, "config", "default-as", identity), and propagate a
cancellable/timeout-bearing context from the caller (RunCmd) or wrap ctx with
context.WithTimeout before calling setDefaultAs; update the RunCmd call site
accordingly and ensure imports for context (and time if you add a timeout) are
added.
---
Duplicate comments:
In `@tests/cli_e2e/core.go`:
- Around line 67-69: The call inside defaultAsInitOnce.Do currently discards the
error from setDefaultAs (called with binaryPath and defaultIdentity); change
this to capture the returned error, handle it by logging or failing the test
setup (e.g., call t.Fatalf or return the error) so CI shows the root
cause—update the anonymous func passed to defaultAsInitOnce.Do to assign err :=
setDefaultAs(binaryPath, defaultIdentity) and then handle err appropriately
rather than ignoring it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4af89c2d-2263-4c6d-b358-fdd375d3de45
📒 Files selected for processing (6)
.github/workflows/cli-e2e.yml.github/workflows/coverage.ymltests/cli_e2e/core.gotests/cli_e2e/demo/task_lifecycle_test.gotests/cli_e2e/task/helpers_test.gotests/cli_e2e/task/tasklist_workflow_test.go
✅ Files skipped from review due to trivial changes (3)
- tests/cli_e2e/task/tasklist_workflow_test.go
- tests/cli_e2e/task/helpers_test.go
- .github/workflows/cli-e2e.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cli_e2e/demo/task_lifecycle_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/cli_e2e/core_test.go (1)
42-59: Working directory change may affect parallel tests.This test changes
os.Getwd()and restores it in a defer. If tests run in parallel (t.Parallel()), this could cause race conditions. Currently the test doesn't callt.Parallel(), so this is safe, but worth noting for future maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/core_test.go` around lines 42 - 59, The test mutates global process state by calling os.Chdir(oldWD)/os.Chdir(testsDir), which can race if tests become parallel; instead avoid changing the process working directory by passing an explicit working-dir into ResolveBinaryPath via the Request object (use Request{Cwd: testsDir}) or, if Request lacks a Cwd field, add a Cwd string to Request and make ResolveBinaryPath prefer it over os.Getwd(); remove the os.Chdir/os.Getwd/oldWD logic and keep t.TempDir(), mustWriteExecutable, t.Setenv and the assertSamePath check..github/workflows/cli-e2e.yml (1)
68-68: Consider pinning gotestsum to a SHA for consistency with other actions and update to the latest version.The workflow pins
actions/checkout,actions/setup-go, andactions/setup-pythonto commit SHAs, butgotestsumuses a version tag (v1.12.3). A newer version (v1.13.0) is available. For supply-chain consistency and to benefit from the latest improvements, consider updating to the latest version and pinning to a commit SHA, or usego installwith a checksum-verified module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/cli-e2e.yml at line 68, Update the gotestsum invocation (the command starting with "go run gotest.tools/gotestsum@v1.12.3") to pin to a reproducible source and to use the newer release: either change the module version to v1.13.0 and replace the version tag with a commit SHA (e.g., gotest.tools/gotestsum@<commitSHA>) or switch to using "go install" with the exact module@version and rely on go.sum for checksum verification; ensure the command that produces cli-e2e-report.xml retains the same flags (--format testname --junitfile cli-e2e-report.xml -- -count=1 -v $packages) while updating only the gotestsum reference.tests/cli_e2e/task/tasklist_workflow_test.go (1)
49-75: Cleanup order differs from the helper function pattern – consider deleting task before tasklist.
t.Cleanupexecutes in LIFO order. Here, task cleanup is registered (line 49) before tasklist cleanup (line 63), so the tasklist delete runs first. However, the helper functions in this test file (createTasklistandcreateTaskin helpers_test.go) register cleanup in the opposite order, where task cleanup runs before tasklist cleanup. Align this test with that pattern for consistency.♻️ Proposed fix to match helper function cleanup order
parentT.Cleanup(func() { deleteResult, deleteErr := clie2e.RunCmd(context.Background(), clie2e.Request{ - Args: []string{"task", "tasks", "delete"}, - Params: map[string]any{"task_guid": taskGUID}, + Args: []string{"task", "tasklists", "delete"}, + Params: map[string]any{"tasklist_guid": tasklistGUID}, }) if deleteErr != nil { - parentT.Errorf("delete task %s: %v", taskGUID, deleteErr) + parentT.Errorf("delete tasklist %s: %v", tasklistGUID, deleteErr) return } if deleteResult.ExitCode != 0 { - parentT.Errorf("delete task %s failed: exit=%d stdout=%s stderr=%s", taskGUID, deleteResult.ExitCode, deleteResult.Stdout, deleteResult.Stderr) + parentT.Errorf("delete tasklist %s failed: exit=%d stdout=%s stderr=%s", tasklistGUID, deleteResult.ExitCode, deleteResult.Stdout, deleteResult.Stderr) } }) parentT.Cleanup(func() { deleteResult, deleteErr := clie2e.RunCmd(context.Background(), clie2e.Request{ - Args: []string{"task", "tasklists", "delete"}, - Params: map[string]any{"tasklist_guid": tasklistGUID}, + Args: []string{"task", "tasks", "delete"}, + Params: map[string]any{"task_guid": taskGUID}, }) if deleteErr != nil { - parentT.Errorf("delete tasklist %s: %v", tasklistGUID, deleteErr) + parentT.Errorf("delete task %s: %v", taskGUID, deleteErr) return } if deleteResult.ExitCode != 0 { - parentT.Errorf("delete tasklist %s failed: exit=%d stdout=%s stderr=%s", tasklistGUID, deleteResult.ExitCode, deleteResult.Stdout, deleteResult.Stderr) + parentT.Errorf("delete task %s failed: exit=%d stdout=%s stderr=%s", taskGUID, deleteResult.ExitCode, deleteResult.Stdout, deleteResult.Stderr) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/task/tasklist_workflow_test.go` around lines 49 - 75, The cleanup registrations are in the wrong order: parentT.Cleanup currently registers task cleanup before tasklist cleanup which causes the tasklist delete to run first (because Cleanup runs LIFO); swap the two parentT.Cleanup blocks so the tasklist cleanup is registered first and the task cleanup (the block referencing taskGUID and calling clie2e.RunCmd with Args {"task","tasks","delete"}) is registered last, matching the helper functions createTasklist/createTask pattern and ensuring the task is deleted before its parent tasklist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/cli-e2e.yml:
- Line 68: Update the gotestsum invocation (the command starting with "go run
gotest.tools/gotestsum@v1.12.3") to pin to a reproducible source and to use the
newer release: either change the module version to v1.13.0 and replace the
version tag with a commit SHA (e.g., gotest.tools/gotestsum@<commitSHA>) or
switch to using "go install" with the exact module@version and rely on go.sum
for checksum verification; ensure the command that produces cli-e2e-report.xml
retains the same flags (--format testname --junitfile cli-e2e-report.xml --
-count=1 -v $packages) while updating only the gotestsum reference.
In `@tests/cli_e2e/core_test.go`:
- Around line 42-59: The test mutates global process state by calling
os.Chdir(oldWD)/os.Chdir(testsDir), which can race if tests become parallel;
instead avoid changing the process working directory by passing an explicit
working-dir into ResolveBinaryPath via the Request object (use Request{Cwd:
testsDir}) or, if Request lacks a Cwd field, add a Cwd string to Request and
make ResolveBinaryPath prefer it over os.Getwd(); remove the
os.Chdir/os.Getwd/oldWD logic and keep t.TempDir(), mustWriteExecutable,
t.Setenv and the assertSamePath check.
In `@tests/cli_e2e/task/tasklist_workflow_test.go`:
- Around line 49-75: The cleanup registrations are in the wrong order:
parentT.Cleanup currently registers task cleanup before tasklist cleanup which
causes the tasklist delete to run first (because Cleanup runs LIFO); swap the
two parentT.Cleanup blocks so the tasklist cleanup is registered first and the
task cleanup (the block referencing taskGUID and calling clie2e.RunCmd with Args
{"task","tasks","delete"}) is registered last, matching the helper functions
createTasklist/createTask pattern and ensuring the task is deleted before its
parent tasklist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35800f1c-3436-45c6-9392-e93df6d22363
📒 Files selected for processing (8)
.github/workflows/cli-e2e.yml.github/workflows/coverage.yml.github/workflows/gitleaks.ymltests/cli_e2e/core.gotests/cli_e2e/core_test.gotests/cli_e2e/demo/task_lifecycle_test.gotests/cli_e2e/task/helpers_test.gotests/cli_e2e/task/tasklist_workflow_test.go
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/gitleaks.yml
- tests/cli_e2e/task/helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/coverage.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/cli_e2e/task/task_status_workflow_test.go (1)
50-53: Inconsistentcompleted_atassertion approach.Line 52 uses
.Int()withNotZero, while line 76 uses.String()comparing to"0". If the API returnscompleted_atas a string (e.g.,"0"),.Int()will return0even for completed tasks with a timestamp string. Use a consistent approach:- assert.NotZero(t, gjson.Get(result.Stdout, "data.task.completed_at").Int(), "stdout:\n%s", result.Stdout) + assert.NotEmpty(t, gjson.Get(result.Stdout, "data.task.completed_at").String(), "stdout:\n%s", result.Stdout) + assert.NotEqual(t, "0", gjson.Get(result.Stdout, "data.task.completed_at").String(), "stdout:\n%s", result.Stdout)Or consistently use
.Int()for both assertions if the field is always numeric.Also applies to: 74-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/task/task_status_workflow_test.go` around lines 50 - 53, The tests inconsistently assert completed_at using gjson .Int() in one place and .String() == "0" in another; pick a single robust approach: read completed_at via gjson.Get(result.Stdout, "data.task.completed_at").String(), convert it to an integer with strconv.ParseInt, assert no parse error, and then assert the parsed value is non-zero (or > 0) using the existing assert helpers (replace the current assert.NotZero and string equality checks). Update both occurrences (the assertion around task completion and the later check at lines referenced) to use this parsed-int approach so both locations are consistent and tolerant of string-encoded timestamps.tests/cli_e2e/core_test.go (1)
348-349:resetDefaultAsInitForTestmutates shared state; ensure no parallel execution.This function directly reassigns the package-level
sync.Once, which is unsafe if tests run concurrently. The current tests don't callt.Parallel(), so this is fine. Consider adding a comment to prevent future contributors from inadvertently enabling parallel execution:func resetDefaultAsInitForTest() { + // WARNING: Not safe for parallel test execution. Do not use t.Parallel() in tests that call this. defaultAsInitOnce = sync.Once{} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/core_test.go` around lines 348 - 349, The resetDefaultAsInitForTest function directly reassigns the package-level sync.Once variable defaultAsInitOnce and therefore mutates shared state; update the code by adding a clear comment above resetDefaultAsInitForTest warning that it is unsafe for parallel test execution (do not call from tests that use t.Parallel()), referencing resetDefaultAsInitForTest and defaultAsInitOnce so future contributors know the constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cli_e2e/task/tasklist_workflow_test.go`:
- Around line 49-75: The two parentT.Cleanup registrations are in the wrong
order for Go's LIFO cleanup: currently the task delete is registered first and
the tasklist delete second, so at teardown the tasklist is deleted before the
task causing potential failures; swap the registrations so the tasklist delete
cleanup is registered first and the task delete cleanup is registered second
(i.e., register clie2e.RunCmd call with Args {"task","tasklists","delete"}
before the clie2e.RunCmd call with Args {"task","tasks","delete"}) so the task
delete runs before its parent tasklist; keep the same error handling/logging for
deleteResult and deleteErr for both cleanup closures.
---
Nitpick comments:
In `@tests/cli_e2e/core_test.go`:
- Around line 348-349: The resetDefaultAsInitForTest function directly reassigns
the package-level sync.Once variable defaultAsInitOnce and therefore mutates
shared state; update the code by adding a clear comment above
resetDefaultAsInitForTest warning that it is unsafe for parallel test execution
(do not call from tests that use t.Parallel()), referencing
resetDefaultAsInitForTest and defaultAsInitOnce so future contributors know the
constraint.
In `@tests/cli_e2e/task/task_status_workflow_test.go`:
- Around line 50-53: The tests inconsistently assert completed_at using gjson
.Int() in one place and .String() == "0" in another; pick a single robust
approach: read completed_at via gjson.Get(result.Stdout,
"data.task.completed_at").String(), convert it to an integer with
strconv.ParseInt, assert no parse error, and then assert the parsed value is
non-zero (or > 0) using the existing assert helpers (replace the current
assert.NotZero and string equality checks). Update both occurrences (the
assertion around task completion and the later check at lines referenced) to use
this parsed-int approach so both locations are consistent and tolerant of
string-encoded timestamps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2929d4de-fef6-4df2-b658-a9f09cf6d103
📒 Files selected for processing (13)
.github/workflows/cli-e2e.yml.github/workflows/coverage.yml.github/workflows/gitleaks.ymltests/cli_e2e/cli-e2e-testcase-writer/SKILL.mdtests/cli_e2e/core.gotests/cli_e2e/core_test.gotests/cli_e2e/demo/task_lifecycle_test.gotests/cli_e2e/task/helpers_test.gotests/cli_e2e/task/task_comment_workflow_test.gotests/cli_e2e/task/task_reminder_workflow_test.gotests/cli_e2e/task/task_status_workflow_test.gotests/cli_e2e/task/tasklist_add_task_workflow_test.gotests/cli_e2e/task/tasklist_workflow_test.go
✅ Files skipped from review due to trivial changes (4)
- .github/workflows/gitleaks.yml
- tests/cli_e2e/demo/task_lifecycle_test.go
- .github/workflows/cli-e2e.yml
- tests/cli_e2e/task/task_reminder_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/coverage.yml
- tests/cli_e2e/task/task_comment_workflow_test.go
- tests/cli_e2e/task/tasklist_add_task_workflow_test.go
- tests/cli_e2e/task/helpers_test.go
liangshuo-1
approved these changes
Apr 3, 2026
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
3 participants
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
Add a CLI E2E testing framework for lark-cli, cover core task workflows with real end-to-end cases, and wire the suite into GitHub Actions so the targeted CLI E2E cases run automatically in CI with bot credentials and test reports.
Changes
Added the tests/cli_e2e E2E test harness, including binary discovery, CLI invocation helpers, shared assertions, and framework self-tests. Also added an initial demo testcase to validate the framework structure and usage.
Added real task-domain CLI E2E coverage and contributor guidance. This includes multiple workflow-style task testcases under tests/cli_e2e/task, helper utilities for setup/cleanup, a cli-e2e-testcase-writer skill for adding future cases consistently, and README documentation for the new E2E module.
Added a dedicated GitHub Actions workflow for CLI E2E. The workflow builds lark-cli, initializes bot credentials from TEST_BOT1_APP_ID / TEST_BOT1_APP_SECRET, runs all tests/cli_e2e packages except the framework root package and demo, uses gotestsum for readable test output, publishes JUnit reports, and skips forked PR runs that cannot access repository secrets.
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit
Tests
Documentation
Chores