Open
Conversation
Change-Id: I62edf59d179e407954f65f82e94cf5dcf4938080
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:
📝 WalkthroughWalkthroughAdds comprehensive CLI end-to-end tests and package-local test helpers across base, calendar, contact, docs, drive, im, sheets, and wiki domains, implementing resource lifecycle workflows, task polling, JSON extraction, CI-aware skipping, and standardized cleanup utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
Greptile SummaryThis PR adds a large suite of CLI E2E tests spanning base, calendar, contact, docs, drive, IM, sheets, and wiki, plus enhances Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/cleanup suggestions that do not affect correctness. All previously flagged blocking issues are resolved: docs/helpers_test.go is present, unused IM helpers were removed, and the calendar no-assertion sub-test is gone. The remaining findings are cleanup hygiene issues that do not affect test correctness or CI reliability in normal conditions. tests/cli_e2e/wiki/wiki_workflow_test.go (no cleanup + flaky page_token assertion) and tests/cli_e2e/drive/helpers_test.go (silent cleanup errors) Important Files Changed
Reviews (8): Last reviewed commit: "test: stabilize cli e2e bot-only coverag..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Change-Id: I4436100c30adf2694cd06953961f8d77f576fc1e
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (26)
tests/cli_e2e/drive/drive_files_workflow_test.go-41-46 (1)
41-46:⚠️ Potential issue | 🟠 MajorMake cleanup bounded and assert deletion success.
Cleanup currently uses
context.Background()and ignores errors/result (Lines 42-45), which can silently leak folders or hang teardown.Proposed fix
parentT.Cleanup(func() { - clie2e.RunCmd(context.Background(), clie2e.Request{ + cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cleanupCancel() + cleanupResult, cleanupErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{ Args: []string{"drive", "files", "delete"}, Params: map[string]any{"file_token": folderToken, "type": "folder"}, }) + require.NoError(parentT, cleanupErr) + cleanupResult.AssertExitCode(parentT, 0) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/drive_files_workflow_test.go` around lines 41 - 46, The cleanup closure uses context.Background() and ignores the RunCmd result, which can hang or silently fail; change the call inside parentT.Cleanup to use parentT.Context() instead of context.Background(), capture the returned result/error from clie2e.RunCmd(Request{Args:[]string{"drive","files","delete"}, Params: map[string]any{"file_token": folderToken, "type":"folder"}}), and assert deletion succeeded (fail the test or log on non-nil error or non-success response). Keep the same Request/Params and reference RunCmd, Request, and folderToken when making the change.tests/cli_e2e/drive/drive_files_workflow_test.go-17-21 (1)
17-21:⚠️ Potential issue | 🟠 MajorAdd config-dir isolation for this E2E test.
This test can read/write CLI config through the real binary path; please isolate it with
t.Setenvat setup (Line 17) to avoid cross-test and local-environment interference.Proposed fix
func TestDrive_FilesCreateFolderWorkflow(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) parentT := t ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel)As per coding guidelines:
**/*_test.go: Isolate config state in Go tests by usingt.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/drive_files_workflow_test.go` around lines 17 - 21, The test TestDrive_FilesCreateFolderWorkflow should isolate CLI config by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at setup (immediately after obtaining parentT or ctx) so the test uses a private config directory; add that t.Setenv call before any CLI binary invocations or config reads in the TestDrive_FilesCreateFolderWorkflow function.tests/cli_e2e/docs/docs_update_test.go-18-22 (1)
18-22:⚠️ Potential issue | 🟠 MajorIsolate CLI config state here as well for deterministic E2E behavior.
This test also relies on global config location; that can leak state across tests/runs.
Suggested fix
func TestDocs_UpdateWorkflow(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) parentT := t ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel)As per coding guidelines,
**/*_test.go: "Isolate config state in Go tests by usingt.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/docs/docs_update_test.go` around lines 18 - 22, The test TestDocs_UpdateWorkflow should isolate CLI config state to avoid leaking across runs: in the TestDocs_UpdateWorkflow function set the environment variable LARKSUITE_CLI_CONFIG_DIR to a temporary directory using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) (or equivalent) before creating the context/cancel so the CLI reads a per-test config dir; ensure this is done near the start of TestDocs_UpdateWorkflow to make E2E behavior deterministic.tests/cli_e2e/docs/docs_create_fetch_test.go-18-22 (1)
18-22:⚠️ Potential issue | 🟠 MajorIsolate CLI config state for this test to avoid cross-test interference.
This test currently uses shared default CLI config state. Please scope config storage to a temp dir within the test.
Suggested fix
func TestDocs_CreateAndFetchWorkflow(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) parentT := t ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel)As per coding guidelines,
**/*_test.go: "Isolate config state in Go tests by usingt.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/docs/docs_create_fetch_test.go` around lines 18 - 22, The test TestDocs_CreateAndFetchWorkflow currently uses shared CLI config; to isolate state, set the LARKSUITE_CLI_CONFIG_DIR env var to a temp dir at the start of the test (e.g., call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) inside TestDocs_CreateAndFetchWorkflow before creating ctx) so the CLI config is scoped to this test and won't interfere with other tests.tests/cli_e2e/sheets/sheets_filter_workflow_test.go-19-23 (1)
19-23:⚠️ Potential issue | 🟠 MajorIsolate config dir in both E2E tests to avoid cross-test contamination.
These tests invoke real CLI flows and should set a per-test config directory; otherwise auth/profile state can leak across tests and produce flaky results.
✅ Suggested patch
func TestSheets_FilterWorkflow(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) parentT := t ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel) @@ func TestSheets_FindWorkflow(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) parentT := t ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel)As per coding guidelines:
**/*_test.go: Isolate config state in Go tests by usingt.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).Also applies to: 176-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/sheets/sheets_filter_workflow_test.go` around lines 19 - 23, The test TestSheets_FilterWorkflow (and the other E2E test block around the same file) must isolate CLI config to avoid cross-test contamination; add a per-test config dir by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) early in each test (e.g., immediately after parentT := t or in the setup for TestSheets_FilterWorkflow and the other test at ~lines 176-180) so the CLI uses a unique config directory for that run and prevents auth/profile state leakage across tests.tests/cli_e2e/sheets/sheets_crud_workflow_test.go-21-24 (1)
21-24:⚠️ Potential issue | 🟠 MajorAdd per-test CLI config isolation to prevent state leakage.
Both top-level tests should isolate CLI config state before invoking commands. Without this, test order and parallel suite activity can leak identity/config and cause flakes.
✅ Suggested patch
func TestSheets_CRUDE2EWorkflow(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) parentT := t ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel) @@ func TestSheets_SpreadsheetsResource(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) parentT := t ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel)As per coding guidelines:
**/*_test.go: Isolate config state in Go tests by usingt.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).Also applies to: 170-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/sheets/sheets_crud_workflow_test.go` around lines 21 - 24, Add per-test CLI config isolation by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of each top-level test in this file (e.g., inside TestSheets_CRUDE2EWorkflow immediately after obtaining parentT/ctx and likewise in the other top-level test in this file) so each test uses an isolated config dir and prevents state leakage between tests.tests/cli_e2e/sheets/sheets_crud_workflow_test.go-150-166 (1)
150-166:⚠️ Potential issue | 🟠 MajorMake export assertion strict; current flow can pass on partial failure.
Line 163 explicitly tolerates an export race and skips
AssertExitCode/AssertStdoutStatus. That can hide real command failures and undermines the “stable E2E” objective.✅ Suggested patch
t.Run("export spreadsheet with +export", func(t *testing.T) { require.NotEmpty(t, spreadsheetToken, "spreadsheet token is required") - - // Export is an async operation; verify it initiates correctly - // The command may have filesystem race issues but the API call succeeds + outputPath := fmt.Sprintf("%s/export.xlsx", t.TempDir()) result, err := clie2e.RunCmd(ctx, clie2e.Request{ Args: []string{ "sheets", "+export", "--spreadsheet-token", spreadsheetToken, "--file-extension", "xlsx", + "--output-path", outputPath, }, }) require.NoError(t, err) - // Export initiates successfully and returns file_token even if there's a temp file race + result.AssertExitCode(t, 0) + result.AssertStdoutStatus(t, true) assert.NotEmpty(t, gjson.Get(result.Stdout, "data.file_token").String(), "export should return file_token, stdout: %s", result.Stdout) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/sheets/sheets_crud_workflow_test.go` around lines 150 - 166, The test currently ignores command failures by tolerating a temp-file race after calling clie2e.RunCmd; update the "export spreadsheet with +export" t.Run to assert the CLI command succeeded before checking data.file_token by invoking the project's run-result assertions (use the existing clie2e result helpers) immediately after clie2e.RunCmd (check result exit code is 0 and stdout status is success) and only then validate gjson.Get(result.Stdout, "data.file_token") is not empty; keep the require.NoError check for err and remove the comment that skips AssertExitCode/AssertStdoutStatus so failures surface reliably.tests/cli_e2e/wiki/wiki_workflow_test.go-123-146 (1)
123-146:⚠️ Potential issue | 🟠 MajorNode lookup only checks one page and can miss the created/copied node.
Both list-node checks assume the target appears in the first page (
page_size: 50). In non-trivial spaces this can flake. Iterate viadata.page_tokenuntil found or exhausted.🔍 Pagination-safe pattern
- result, err := clie2e.RunCmd(ctx, clie2e.Request{ - Args: []string{"wiki", "nodes", "list"}, - DefaultAs: "bot", - Params: map[string]any{ - "space_id": spaceID, - "page_size": 50, - }, - }) - require.NoError(t, err) - if result.ExitCode != 0 { - skipIfWikiUnavailable(t, result, "requires bot wiki node list capability") - } - result.AssertExitCode(t, 0) - result.AssertStdoutStatus(t, 0) - - nodeItem := gjson.Get(result.Stdout, `data.items.#(node_token=="`+createdNodeToken+`")`) + pageToken := "" + var nodeItem gjson.Result + for { + params := map[string]any{"space_id": spaceID, "page_size": 50} + if pageToken != "" { + params["page_token"] = pageToken + } + result, err := clie2e.RunCmd(ctx, clie2e.Request{ + Args: []string{"wiki", "nodes", "list"}, + DefaultAs: "bot", + Params: params, + }) + require.NoError(t, err) + if result.ExitCode != 0 { + skipIfWikiUnavailable(t, result, "requires bot wiki node list capability") + } + result.AssertExitCode(t, 0) + result.AssertStdoutStatus(t, 0) + + nodeItem = gjson.Get(result.Stdout, `data.items.#(node_token=="`+createdNodeToken+`")`) + if nodeItem.Exists() { + break + } + pageToken = gjson.Get(result.Stdout, "data.page_token").String() + if pageToken == "" { + break + } + } assert.True(t, nodeItem.Exists(), "stdout:\n%s", result.Stdout)Also applies to: 172-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/wiki/wiki_workflow_test.go` around lines 123 - 146, The current test only checks the first page of "wiki nodes list" and can miss the created/copied node; modify the test that runs clie2e.RunCmd with Args {"wiki","nodes","list"} to loop over pagination by inspecting result.Stdout.data.page_token and re-calling clie2e.RunCmd (preserving page_size and space_id) until either a matching node (use gjson query for node_token == createdNodeToken) is found or page_token is empty; update the assertion logic around nodeItem (and mirror the same pagination-safe change for the other similar block that checks nodes) so the test only fails if the node is not found after exhausting pages.tests/cli_e2e/wiki/wiki_workflow_test.go-17-20 (1)
17-20:⚠️ Potential issue | 🟠 MajorIsolate CLI config per test to avoid cross-test state leakage.
This test should set its own config directory before invoking CLI commands; otherwise shared state can make E2E outcomes order-dependent.
✅ Minimal fix
func TestWiki_NodeWorkflow(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel)As per coding guidelines: Isolate config state in Go tests by using
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/wiki/wiki_workflow_test.go` around lines 17 - 20, The test TestWiki_NodeWorkflow uses global CLI config and can leak state between tests; fix it by setting an isolated config dir at the start of the test (before any CLI invocations) using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) inside the TestWiki_NodeWorkflow function (ensure this runs immediately after ctx/cancel setup and before calling any CLI helper or command).tests/cli_e2e/base/helpers_test.go-123-143 (1)
123-143:⚠️ Potential issue | 🟠 Major
createBaselacks cleanup because a+base-deletecommand is not available.Unlike
createTable,createField, and other helpers that registert.Cleanupto delete resources,createBasecannot implement cleanup—there is no+base-deletecommand in the CLI. This inconsistency means bases will accumulate in the test environment. Either add support for base deletion or document why bases should not be cleaned up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/base/helpers_test.go` around lines 123 - 143, createBase currently creates bases without registering t.Cleanup (unlike helpers such as createTable/createField) because there is no +base-delete CLI command; either add CLI support for base deletion and update createBase to call clie2e.RunCmd(... "+base-delete" ...) inside t.Cleanup to remove the created base (use the returned baseToken to identify the base), or if deletion is intentionally unsupported add a clear comment and test-suite documentation explaining why bases are not cleaned up and the expected lifecycle; reference the createBase function, t.Cleanup, and the missing "+base-delete" command when implementing the fix.tests/cli_e2e/drive/drive_permission_user_workflow_test.go-84-97 (1)
84-97:⚠️ Potential issue | 🟠 MajorAdd retry/backoff for
subscription_statusverification.Immediate status checks after subscribe can intermittently fail due to propagation delay. Poll with a short bounded retry window before asserting data presence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/drive_permission_user_workflow_test.go` around lines 84 - 97, The test's immediate check of subscription_status can race with propagation; wrap the clie2e.RunCmd call for "drive user subscription_status" in a short bounded retry loop with exponential/backoff sleeps (e.g., 3–5 attempts, increasing delays) before asserting result.AssertExitCode and require.NotEmpty on the gjson.Get(..., "data") result. Retry using the same ctx, Args and Params (eventType) and break early on success; after exhausting attempts fail the test with the final stdout. Ensure the retry surrounds the RunCmd invocation and the subsequent gjson.Get/require.NotEmpty check so transient empty responses are retried.tests/cli_e2e/drive/helpers_test.go-96-105 (1)
96-105:⚠️ Potential issue | 🟠 MajorSingle-shot import polling is flaky for async completion.
For async import, one
+task_resultcall is not enough for deterministic tests. Poll with bounded retry until token is present (or context timeout), then assert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/helpers_test.go` around lines 96 - 105, The single-shot poll using clie2e.RunCmd for "+task_result" is flaky; change the block that runs clie2e.RunCmd (where ticket != "" and currently assigns pollResult, pollErr, pollResult.AssertExitCode and docToken) to loop with a bounded retry/backoff until gjson.Get(pollResult.Stdout, "data.token").String() yields a non-empty docToken or the ctx times out; on each iteration call clie2e.RunCmd with clie2e.Request{Args: []string{"drive", "+task_result", "--ticket", ticket, "--scenario", "import"}}, check pollErr and exit code, sleep briefly between attempts (or use exponential backoff), then after the loop assert docToken is non-empty (require.NotEmpty/require.True) so the test deterministically waits for async completion.tests/cli_e2e/drive/drive_move_workflow_test.go-41-53 (1)
41-53:⚠️ Potential issue | 🟠 Major
task_checkpath should poll until terminal state.This does only one
+task_resultcheck; move tasks can still be in progress. Add bounded polling and assert terminal success (ready=true,failed=false) before passing the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/drive_move_workflow_test.go` around lines 41 - 53, The test currently performs a single clie2e.RunCmd call for the "+task_result" path using taskID (in the drive_move_workflow_test.go block) which can miss in-progress move tasks; replace the single check with a bounded retry loop that repeatedly calls clie2e.RunCmd for the same Args (including "--task-id" taskID and "--scenario" "task_check") until a terminal state or timeout is reached, asserting each RunCmd succeeds, and finally assert the terminal result indicates success by verifying the task JSON fields (e.g., ready==true and failed==false) instead of only using taskResult.AssertStdoutStatus; ensure the loop has a reasonable max retries and sleep between attempts to avoid flakiness.tests/cli_e2e/drive/helpers_test.go-30-32 (1)
30-32:⚠️ Potential issue | 🟠 MajorUse collision-proof temp filenames for e2e stability.
Second-resolution timestamps can collide across parallel/subsequent runs and cause flaky overwrites. Use
os.CreateTempin the fixture directory instead.Suggested fix
- fileName := suffix + "-" + time.Now().UTC().Format("20060102-150405") + ".txt" - filePath := filepath.Join(testDir, fileName) - err = os.WriteFile(filePath, []byte(content), 0o644) + f, err := os.CreateTemp(testDir, suffix+"-*.txt") + require.NoError(t, err) + filePath := f.Name() + _, err = f.WriteString(content) + require.NoError(t, err) + require.NoError(t, f.Close())- fileName := "drive-e2e-" + suffix + "-" + time.Now().UTC().Format("20060102-150405") + ".md" - mdFile := filepath.Join(testDir, fileName) - err = os.WriteFile(mdFile, []byte(content), 0o644) + f, err := os.CreateTemp(testDir, "drive-e2e-"+suffix+"-*.md") + require.NoError(t, err) + mdFile := f.Name() + _, err = f.WriteString(content) + require.NoError(t, err) + require.NoError(t, f.Close())Based on learnings: "In larksuite/cli Go tests (files matching *_test.go), when creating or populating test fixtures under t.TempDir(), use the standard library filesystem APIs: os.Mkdir, os.Create, os.CreateTemp, and os.WriteFile."
Also applies to: 78-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/helpers_test.go` around lines 30 - 32, The current test creates a file with a timestamp-based name (fileName/filePath) then writes it via os.WriteFile, which can collide; replace that pattern by using os.CreateTemp in the fixture directory (e.g., call os.CreateTemp(testDir, suffix+"-*.txt") to get a unique temp file), write the content to the returned *os.File (or use os.WriteFile on the created path) and close the file; update both occurrences referenced (the block creating fileName/filePath around lines shown and the similar block at 78-80) so tests use os.CreateTemp instead of timestamp names.tests/cli_e2e/calendar/calendar_manage_calendar_test.go-17-20 (1)
17-20:⚠️ Potential issue | 🟠 MajorMissing per-test config isolation.
Please isolate CLI config path for this test to avoid cross-test coupling.
As per coding guidelines, `Isolate config state in Go tests by using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())`.🔧 Proposed fix
func TestCalendar_ManageCalendar(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/calendar/calendar_manage_calendar_test.go` around lines 17 - 20, TestCalendar_ManageCalendar currently shares global CLI config state across tests; update the test setup in TestCalendar_ManageCalendar to isolate config by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before any CLI usage (e.g., right after creating ctx/cancel or at start of the test) so the test uses a unique temporary config directory and avoids cross-test coupling.tests/cli_e2e/calendar/calendar_view_agenda_test.go-16-19 (1)
16-19:⚠️ Potential issue | 🟠 MajorSet isolated config directory for test stability.
This test should isolate config state to prevent interference from other tests.
As per coding guidelines, `Isolate config state in Go tests by using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())`.🔧 Proposed fix
func TestCalendar_ViewAgenda(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/calendar/calendar_view_agenda_test.go` around lines 16 - 19, In TestCalendar_ViewAgenda, isolate CLI config state by setting the LARKSUITE_CLI_CONFIG_DIR env var to a temp dir at the start of the test (use t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())); add this immediately after creating ctx/cancel so TestCalendar_ViewAgenda uses a unique config directory and avoids cross-test interference.tests/cli_e2e/calendar/helpers_test.go-39-53 (1)
39-53:⚠️ Potential issue | 🟠 MajorBound cleanup delete with a timeout.
Using
context.Background()in teardown can hang the suite if the delete command blocks. Use a short timeout context for cleanup.🔧 Proposed fix
parentT.Cleanup(func() { - deleteResult, deleteErr := clie2e.RunCmd(context.Background(), clie2e.Request{ + cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cleanupCancel() + deleteResult, deleteErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{ Args: []string{"calendar", "events", "delete"}, Params: map[string]any{ "calendar_id": calendarID, "event_id": eventID, }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/calendar/helpers_test.go` around lines 39 - 53, The cleanup uses context.Background() when calling clie2e.RunCmd inside parentT.Cleanup which can block; change to a short timeout context by creating a context with context.WithTimeout (e.g., 5–10s) and pass that ctx to clie2e.RunCmd, ensure you call the cancel function in a defer, and handle a timeout error path (log or report that the delete timed out) before checking deleteResult.ExitCode so the teardown cannot hang indefinitely.tests/cli_e2e/calendar/calendar_find_meeting_time_test.go-16-19 (1)
16-19:⚠️ Potential issue | 🟠 MajorIsolate CLI config state for this E2E test.
Add per-test config isolation before running commands to avoid state bleed across tests.
As per coding guidelines, `Isolate config state in Go tests by using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())`.🔧 Proposed fix
func TestCalendar_FindMeetingTime(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/calendar/calendar_find_meeting_time_test.go` around lines 16 - 19, In TestCalendar_FindMeetingTime, isolate CLI config state by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) early in the test (after ctx/cancel setup and before any CLI invocations) so each run uses a unique temporary config directory; modify the TestCalendar_FindMeetingTime function to add this t.Setenv(...) line (use t.TempDir() to create the directory) to prevent state bleed across tests.tests/cli_e2e/calendar/calendar_create_event_test.go-18-21 (1)
18-21:⚠️ Potential issue | 🟠 MajorAdd config isolation at test start.
This test should set an isolated CLI config directory to avoid test state leakage.
As per coding guidelines, `Isolate config state in Go tests by using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())`.🔧 Proposed fix
func TestCalendar_CreateEvent(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/calendar/calendar_create_event_test.go` around lines 18 - 21, In TestCalendar_CreateEvent, set an isolated CLI config directory at the start to avoid state leakage by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) right after creating the test context (in the TestCalendar_CreateEvent function) so the test uses a temp config directory and does not share global CLI state.tests/cli_e2e/calendar/calendar_manage_calendar_test.go-44-99 (1)
44-99:⚠️ Potential issue | 🟠 MajorRegister best-effort cleanup immediately after calendar creation.
Deletion only in the final subtest is brittle; failures/timeouts earlier can leave orphan calendars.
🔧 Proposed fix
var createdCalendarID string + var calendarDeleted bool @@ t.Run("create calendar", func(t *testing.T) { result, err := clie2e.RunCmd(ctx, clie2e.Request{ @@ createdCalendarID = gjson.Get(result.Stdout, "data.calendar.calendar_id").String() require.NotEmpty(t, createdCalendarID) + + t.Cleanup(func() { + if createdCalendarID == "" || calendarDeleted { + return + } + cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cleanupCancel() + _, _ = clie2e.RunCmd(cleanupCtx, clie2e.Request{ + Args: []string{"calendar", "calendars", "delete"}, + Params: map[string]any{ + "calendar_id": createdCalendarID, + }, + }) + }) }) @@ t.Run("delete calendar", func(t *testing.T) { @@ require.NoError(t, err) result.AssertExitCode(t, 0) result.AssertStdoutStatus(t, 0) + calendarDeleted = true })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/calendar/calendar_manage_calendar_test.go` around lines 44 - 99, The create-calendar subtest registers the calendar ID in createdCalendarID but only deletes it later, which can leave orphans if earlier steps fail; immediately after successfully creating the calendar in the "create calendar" test (where createdCalendarID is set from gjson.Get(result.Stdout, "data.calendar.calendar_id")), register a cleanup using t.Cleanup (or defer inside the subtest) that calls the same clie2e.RunCmd with Args {"calendar","calendars","delete"} and Params {"calendar_id": createdCalendarID} so the calendar is removed even if subsequent subtests fail; update references in the "create calendar" block to ensure createdCalendarID is captured and cleaned up deterministically.tests/cli_e2e/im/chat_workflow_test.go-192-195 (1)
192-195:⚠️ Potential issue | 🟠 MajorDo not print share links in test logs.
share_linkis an access-bearing URL and should not be emitted to CI logs.Suggested fix
shareLink := gjson.Get(result.Stdout, "data.share_link").String() require.NotEmpty(t, shareLink, "share_link should not be empty") - t.Logf("Generated share link: %s", shareLink) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/chat_workflow_test.go` around lines 192 - 195, The test currently logs the access-bearing URL via the variable shareLink and t.Logf("Generated share link: %s", shareLink); remove the sensitive output by deleting the t.Logf call (or replace it with a non-sensitive assertion/log such as require.NotEmpty(t, shareLink) and a generic t.Logf("share link generated")/no details). Locate the code that assigns shareLink and the t.Logf invocation in the test (shareLink := gjson.Get(result.Stdout, "data.share_link").String() and t.Logf(...)) and ensure no direct share_link value is printed to CI logs.tests/cli_e2e/im/message_workflow_test.go-39-40 (1)
39-40:⚠️ Potential issue | 🟠 MajorAssert the fetched batch contains the requested message ID.
require.NotEmpty(data)is too weak; this can pass even when the target message is missing. Validate that one returned item matchesmessageID.Suggested fix
messages := gjson.Get(result.Stdout, "data").Array() require.NotEmpty(t, messages, "should get at least one message") + found := false + for _, m := range messages { + if m.Get("message_id").String() == messageID { + found = true + break + } + } + require.True(t, found, "expected message_id %s in mget response", messageID) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/message_workflow_test.go` around lines 39 - 40, The test currently only asserts messages := gjson.Get(result.Stdout, "data").Array() and require.NotEmpty(t, messages) which can pass even if the requested messageID is absent; update the assertion to verify at least one returned object has id == messageID by iterating messages (or using a gjson path) and asserting a match for messageID (referencing variables messages, result.Stdout and messageID in message_workflow_test.go) so the fetched batch actually contains the requested message.tests/cli_e2e/im/chat_workflow_test.go-115-137 (1)
115-137:⚠️ Potential issue | 🟠 MajorValidate update effects, not just command success.
The test currently passes even if
+chat-updatereturns success but does not persist changes. Add a follow-upim chats getassertion for updatednameanddescription.Suggested enhancement
t.Run("update chat description", func(t *testing.T) { result, err := clie2e.RunCmd(ctx, clie2e.Request{ Args: []string{"im", "+chat-update", "--chat-id", chatID, "--description", updatedDescription, }, }) require.NoError(t, err) result.AssertExitCode(t, 0) result.AssertStdoutStatus(t, true) + + getResult, getErr := clie2e.RunCmd(ctx, clie2e.Request{ + Args: []string{"im", "chats", "get"}, + Params: map[string]any{"chat_id": chatID}, + }) + require.NoError(t, getErr) + getResult.AssertExitCode(t, 0) + getResult.AssertStdoutStatus(t, 0) + require.Equal(t, updatedName, gjson.Get(getResult.Stdout, "data.name").String()) + require.Equal(t, updatedDescription, gjson.Get(getResult.Stdout, "data.description").String()) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/chat_workflow_test.go` around lines 115 - 137, After calling clie2e.RunCmd for "+chat-update" (the tests that update name and description), add a follow-up verification by running clie2e.RunCmd with Args for "im", "chats", "get", "--chat-id", chatID and assert the returned chat's fields match the updated values; specifically, after the "+chat-update" that uses updatedName call "im chats get" and assert the response contains updatedName, and after the "+chat-update" that uses updatedDescription call "im chats get" and assert the response contains updatedDescription, using the same result.AssertExitCode/assertions pattern to ensure persistence.tests/cli_e2e/im/helpers_test.go-169-171 (1)
169-171:⚠️ Potential issue | 🟠 MajorIncrease suffix entropy to avoid flaky name collisions.
generateSuffix()is second-granular, so parallel/fast E2E runs can generate identical resource names and intermittently fail with “already exists” conflicts.Suggested fix
import ( "context" + "fmt" "testing" "time" @@ func generateSuffix() string { - return time.Now().UTC().Format("20060102-150405") + now := time.Now().UTC() + return fmt.Sprintf("%s-%09d", now.Format("20060102-150405"), now.Nanosecond()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/helpers_test.go` around lines 169 - 171, The suffix generation in generateSuffix() is only second-granular causing collisions; update generateSuffix() to add higher entropy (e.g. append time.Now().UTC().Format("20060102-150405.000000000") or include time.Now().UnixNano() and/or a short cryptographic or math/rand alphanumeric segment) so names are unique across parallel fast E2E runs; ensure tests that consume generateSuffix() continue to accept the new format.tests/cli_e2e/im/message_workflow_test.go-58-81 (1)
58-81:⚠️ Potential issue | 🟠 MajorVerify reply payload integrity (
data.message_id).Both reply subtests only check command success. They should also assert non-empty returned
data.message_id(and ideally that it differs from the originalmessageID).Suggested fix
t.Run("reply to message with text", func(t *testing.T) { result, err := clie2e.RunCmd(ctx, clie2e.Request{ Args: []string{"im", "+messages-reply", "--message-id", messageID, "--text", replyText, }, }) require.NoError(t, err) result.AssertExitCode(t, 0) result.AssertStdoutStatus(t, true) + replyID := gjson.Get(result.Stdout, "data.message_id").String() + require.NotEmpty(t, replyID, "reply message_id should not be empty") + require.NotEqual(t, messageID, replyID, "reply should produce a new message_id") }) @@ t.Run("reply to message with markdown", func(t *testing.T) { markdownReply := "**Bold** reply" result, err := clie2e.RunCmd(ctx, clie2e.Request{ Args: []string{"im", "+messages-reply", "--message-id", messageID, "--markdown", markdownReply, }, }) require.NoError(t, err) result.AssertExitCode(t, 0) result.AssertStdoutStatus(t, true) + replyID := gjson.Get(result.Stdout, "data.message_id").String() + require.NotEmpty(t, replyID, "reply message_id should not be empty") + require.NotEqual(t, messageID, replyID, "reply should produce a new message_id") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/message_workflow_test.go` around lines 58 - 81, The tests "reply to message with text" and "reply to message with markdown" currently only assert command success; add assertions that the CLI response contains a non-empty reply message id and that it differs from the original messageID. After calling clie2e.RunCmd (in both subtests where variables replyText and markdownReply are used), parse the command output/result to extract data.message_id (or use result helper methods if available), assert it is not empty and assert it is not equal to messageID; keep these checks alongside the existing Require/AssertExitCode and AssertStdoutStatus calls to ensure payload integrity.tests/cli_e2e/im/chat_workflow_test.go-17-21 (1)
17-21:⚠️ Potential issue | 🟠 MajorAdd config-dir isolation to prevent polluting real user CLI state.
These tests invoke the real CLI binary via
exec.CommandContext(), which exercises the actualNewDefault()factory path that reads filesystem config. Without isolation, they will use and potentially mutate the real user's~/.config/lark-clistate.Add this near the top of each test function (or in a test helper called from each test):
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())This isolates config to a temporary directory per test, following the required pattern for Go tests in this codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/chat_workflow_test.go` around lines 17 - 21, The test TestIM_ChatCreateSendWorkflow invokes the real CLI via exec.CommandContext() which triggers NewDefault() to read/write the real user's config; to isolate state, set the environment variable LARKSUITE_CLI_CONFIG_DIR to a per-test temp directory at the start of the test (or in a helper called by the test) using t.Setenv with t.TempDir() so the CLI uses an isolated config dir and cannot mutate real user config.
🟡 Minor comments (2)
tests/cli_e2e/drive/helpers_test.go-28-33 (1)
28-33:⚠️ Potential issue | 🟡 MinorHandle fixture directory creation errors explicitly.
Both setup paths ignore
os.MkdirAllerrors, which can hide the real setup failure and produce noisier downstream assertions.Suggested fix
- _ = os.MkdirAll(testDir, 0755) + err := os.MkdirAll(testDir, 0o755) + require.NoError(t, err) ... - err := os.WriteFile(filePath, []byte(content), 0644) + err = os.WriteFile(filePath, []byte(content), 0o644) require.NoError(t, err)- _ = os.MkdirAll(testDir, 0755) + err := os.MkdirAll(testDir, 0o755) + require.NoError(t, err) ... - err := os.WriteFile(mdFile, []byte(content), 0644) + err = os.WriteFile(mdFile, []byte(content), 0o644) require.NoError(t, err)Also applies to: 75-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/helpers_test.go` around lines 28 - 33, The setup is ignoring errors from os.MkdirAll(testDir, 0755); update both occurrences (the one creating testDir and the one at lines ~75-81) to capture the returned error and assert it using require.NoError(t, err) (or fail the test via t.Fatalf) so directory creation failures are reported immediately; look for the os.MkdirAll calls and add error handling around them referencing testDir and the test's t variable.tests/cli_e2e/calendar/calendar_create_event_test.go-95-107 (1)
95-107:⚠️ Potential issue | 🟡 Minor
verify delete acknowledgedis effectively a no-op.This block has no assertion on deletion behavior, so it can pass without validating anything meaningful.
🔧 Minimal fix (remove no-op check)
- // Step 5: Verify delete was acknowledged (event may have eventual consistency) - t.Run("verify delete acknowledged", func(t *testing.T) { - require.NotEmpty(t, eventID) - result, err := clie2e.RunCmd(ctx, clie2e.Request{ - Args: []string{"calendar", "events", "get"}, - Params: map[string]any{ - "calendar_id": calendarID, - "event_id": eventID, - }, - }) - require.NoError(t, err) - // Note: API may have eventual consistency - delete acknowledged but get may still succeed briefly - _ = result - }) + // Step 5 removed: previous check had no assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/calendar/calendar_create_event_test.go` around lines 95 - 107, The test block named "verify delete acknowledged" is a no-op and should either be removed or replaced with a real assertion: call clie2e.RunCmd (as currently done) with calendar_id and event_id and then assert the delete was acknowledged by checking the response or error (e.g. require.Error(t, err) or require.Contains(t, result.String(), "not found") / assert HTTP-not-found in the result) using eventID and calendarID; update the t.Run body to assert the expected "not found" or deletion-acknowledged signal instead of discarding result.
🧹 Nitpick comments (5)
tests/cli_e2e/wiki/helpers_test.go (1)
21-36: Make JSON extraction resilient when stdout is non-empty but non-JSON.Current logic only checks stderr when stdout is empty. If stdout has noise and stderr contains the JSON error envelope, this helper fails and skip handling won’t run.
♻️ Suggested hardening
func wikiJSONPayload(t *testing.T, result *clie2e.Result) string { t.Helper() - raw := strings.TrimSpace(result.Stdout) - if raw == "" { - raw = strings.TrimSpace(result.Stderr) - } - - start := strings.LastIndex(raw, "\n{") - if start >= 0 { - start++ - } else { - start = strings.Index(raw, "{") - } - require.NotEqualf(t, -1, start, "json payload not found:\nstdout:\n%s\nstderr:\n%s", result.Stdout, result.Stderr) - - payload := raw[start:] - require.Truef(t, gjson.Valid(payload), "invalid json payload:\n%s", payload) - return payload + extract := func(raw string) (string, bool) { + if raw == "" { + return "", false + } + start := strings.LastIndex(raw, "\n{") + if start >= 0 { + start++ + } else { + start = strings.Index(raw, "{") + } + if start < 0 { + return "", false + } + payload := raw[start:] + return payload, gjson.Valid(payload) + } + + if payload, ok := extract(strings.TrimSpace(result.Stdout)); ok { + return payload + } + if payload, ok := extract(strings.TrimSpace(result.Stderr)); ok { + return payload + } + require.Failf(t, "json payload not found", "stdout:\n%s\nstderr:\n%s", result.Stdout, result.Stderr) + return "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/wiki/helpers_test.go` around lines 21 - 36, The JSON extraction currently only falls back to stderr when stdout is empty, causing failures if stdout contains non-JSON noise and stderr holds the JSON envelope; update the logic around raw/result to try both outputs: first attempt to extract and validate a JSON payload from result.Stdout (using the existing LastIndex/Index logic and gjson.Valid), and if that yields no valid JSON, repeat the same extraction on result.Stderr, then require a payload; refer to the variables and helpers like result.Stdout, result.Stderr, raw, start, payload, and gjson.Valid when implementing the fallback.tests/cli_e2e/base/base_role_workflow_test.go (1)
118-129: Double-deletion: explicit delete subtest +createRolecleanup handler.The
createRolehelper (line 35) registers at.Cleanuphandler that will delete the role. However, this "delete" subtest also explicitly deletes the same role. This results in:
- Subtest deletes the role
- Cleanup handler attempts to delete the already-deleted role
While
isCleanupSuppressedResulthandles "not found" errors gracefully, this pattern is unclear. Consider one of:
- Remove the explicit "delete" subtest since cleanup handles it
- Don't use
createRolefor this test and manage the lifecycle manually- Add a mechanism to disable the cleanup handler after explicit deletion
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/base/base_role_workflow_test.go` around lines 118 - 129, The test double-deletes the role because createRole registers a t.Cleanup that deletes the role, while the "delete" subtest also deletes it; remove the explicit "delete" t.Run block (the subtest that calls clie2e.RunCmd with "+role-delete") and rely on the createRole cleanup, keeping the createRole helper and its t.Cleanup intact so the role lifecycle is managed in one place.tests/cli_e2e/base/helpers_test.go (1)
510-522: Consider usingt.TempDir()instead of the working directory for temporary files.
writeTempAttachmentwrites files toos.Getwd()(the current working directory). Usingt.TempDir()would:
- Ensure automatic cleanup even if the test panics before the
t.Cleanupruns- Avoid potential filename collisions in parallel test runs
- Keep the working directory clean
♻️ Suggested refactor
func writeTempAttachment(t *testing.T, content string) string { t.Helper() - wd, err := os.Getwd() - require.NoError(t, err) - - path := filepath.Join(wd, "attachment-"+testSuffix()+".txt") + path := filepath.Join(t.TempDir(), "attachment-"+testSuffix()+".txt") err = os.WriteFile(path, []byte(content), 0o644) require.NoError(t, err) - t.Cleanup(func() { - _ = os.Remove(path) - }) - return "./" + filepath.Base(path) + return path }Note: If the CLI requires a relative path specifically, you may need to adjust the return value accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/base/helpers_test.go` around lines 510 - 522, The helper writeTempAttachment currently writes files into os.Getwd(); change it to create the file inside the test-specific directory returned by t.TempDir() to avoid collisions and ensure automatic cleanup: use t.TempDir() to build the file path (instead of os.Getwd()), write the content there, register no extra cleanup (t.TempDir is auto-removed), and if the CLI requires a relative path, return the path relative to the current working directory (e.g., "./"+filepath.Base(path)) or adjust tests to accept absolute paths; update references in tests that call writeTempAttachment accordingly.tests/cli_e2e/drive/drive_permission_user_workflow_test.go (1)
99-110: Consider asserting post-remove state for full lifecycle coverage.After
remove_subscription, add a follow-upsubscription_statuscheck (with bounded retry) to confirm unsubscription actually took effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/drive_permission_user_workflow_test.go` around lines 99 - 110, After the "remove subscription" t.Run block that calls clie2e.RunCmd with Args ["drive","user","remove_subscription"], add a follow-up check that calls clie2e.RunCmd for the "subscription_status" command (or equivalent endpoint) to verify the subscription is removed; implement a bounded retry loop (e.g., fixed attempts with short sleeps) around the subscription_status call to tolerate eventual consistency, assert exit code 0 and that the returned status indicates unsubscribed, and fail the test if the retry window expires—use the same ctx, clie2e.Request pattern and Result assertions (result.AssertExitCode / result.AssertStdoutStatus or JSON status assertions) so the lifecycle is fully validated.tests/cli_e2e/im/chat_workflow_test.go (1)
28-57: Reuse existing message helpers to reduce duplicated E2E plumbing.This block repeats the exact
RunCmd + assert + extract message_idlogic already centralized insendMessage/sendMarkdownfromtests/cli_e2e/im/helpers_test.go.Suggested refactor
t.Run("send text message to chat", func(t *testing.T) { - result, err := clie2e.RunCmd(ctx, clie2e.Request{ - Args: []string{"im", "+messages-send", - "--chat-id", chatID, - "--text", messageText, - }, - }) - require.NoError(t, err) - result.AssertExitCode(t, 0) - result.AssertStdoutStatus(t, true) - - messageID := gjson.Get(result.Stdout, "data.message_id").String() + messageID := sendMessage(t, parentT, ctx, chatID, messageText) require.NotEmpty(t, messageID, "message_id should not be empty") }) t.Run("send markdown message to chat", func(t *testing.T) { markdownContent := "**Bold** and *italic* text" - result, err := clie2e.RunCmd(ctx, clie2e.Request{ - Args: []string{"im", "+messages-send", - "--chat-id", chatID, - "--markdown", markdownContent, - }, - }) - require.NoError(t, err) - result.AssertExitCode(t, 0) - result.AssertStdoutStatus(t, true) - - messageID := gjson.Get(result.Stdout, "data.message_id").String() + messageID := sendMarkdown(t, parentT, ctx, chatID, markdownContent) require.NotEmpty(t, messageID, "message_id should not be empty") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/chat_workflow_test.go` around lines 28 - 57, The two test subcases duplicate RunCmd/assert/extract logic; replace them with calls to the centralized helpers sendMessage and sendMarkdown from tests/cli_e2e/im/helpers_test.go: for the "send text message to chat" case call sendMessage(ctx, t, clie2e, chatID, messageText) (or the helper's exact signature) and assert the returned message ID is non-empty; for the "send markdown message to chat" case call sendMarkdown(ctx, t, clie2e, chatID, markdownContent) and similarly assert the returned message ID is non-empty, removing the duplicated RunCmd / require / result handling blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52e5ca1e-ad67-44dc-a364-c504d40548b4
📒 Files selected for processing (22)
tests/cli_e2e/base/base_basic_workflow_test.gotests/cli_e2e/base/base_role_workflow_test.gotests/cli_e2e/base/helpers_test.gotests/cli_e2e/calendar/calendar_create_event_test.gotests/cli_e2e/calendar/calendar_find_meeting_time_test.gotests/cli_e2e/calendar/calendar_manage_calendar_test.gotests/cli_e2e/calendar/calendar_view_agenda_test.gotests/cli_e2e/calendar/helpers_test.gotests/cli_e2e/contact/contact_shortcut_test.gotests/cli_e2e/docs/docs_create_fetch_test.gotests/cli_e2e/docs/docs_update_test.gotests/cli_e2e/drive/drive_files_workflow_test.gotests/cli_e2e/drive/drive_move_workflow_test.gotests/cli_e2e/drive/drive_permission_user_workflow_test.gotests/cli_e2e/drive/helpers_test.gotests/cli_e2e/im/chat_workflow_test.gotests/cli_e2e/im/helpers_test.gotests/cli_e2e/im/message_workflow_test.gotests/cli_e2e/sheets/sheets_crud_workflow_test.gotests/cli_e2e/sheets/sheets_filter_workflow_test.gotests/cli_e2e/wiki/helpers_test.gotests/cli_e2e/wiki/wiki_workflow_test.go
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/cli_e2e/im/helpers_test.go (1)
19-19: No-op cleanup + extra parameter make the helper API misleading.
createChatregisters an empty cleanup callback, andsendMessageacceptsparentTbut doesn’t use it. Consider removingparentTfrom both helpers (or wiring a real cleanup path).♻️ Proposed refactor
-func createChat(t *testing.T, parentT *testing.T, ctx context.Context, name string) string { +func createChat(t *testing.T, ctx context.Context, name string) string { @@ - parentT.Cleanup(func() { - // Best-effort cleanup - chat will be automatically orphaned - // since im chats delete command is not available - }) - return chatID } @@ -func sendMessage(t *testing.T, parentT *testing.T, ctx context.Context, chatID string, text string) string { +func sendMessage(t *testing.T, ctx context.Context, chatID string, text string) string {Also applies to: 35-38, 44-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/im/helpers_test.go` at line 19, The helper API is misleading because createChat registers a no-op cleanup and sendMessage accepts an unused parentT parameter; update the helpers by removing the unused parentT parameter from both createChat and sendMessage and delete the RegisterCleanup no-op call in createChat (or alternatively implement a real cleanup flow and wire parentT into that cleanup) so the function signatures and behavior accurately reflect their responsibilities; locate createChat and sendMessage in the tests/cli_e2e/im/helpers_test.go file to make these changes.
🤖 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/drive/helpers_test.go`:
- Around line 60-64: The cleanup currently calls parentT.Cleanup with an
unbounded context and ignores errors from clie2e.RunCmd; update the cleanup
closure used with parentT.Cleanup to create a context with timeout (e.g.,
context.WithTimeout) and call clie2e.RunCmd with that context, capture its
returned error and assert/fail if non-nil so teardown failures are visible;
apply the same change to the other cleanup block around lines 109-113 and
reference the clie2e.Request construction and the clie2e.RunCmd invocation to
ensure the deletion command is run with a bounded context and checked result.
- Around line 30-32: Replace the manual timestamp-based filename and
os.WriteFile call with os.CreateTemp to avoid collisions: create a temp file in
testDir using os.CreateTemp(testDir, suffix+"-*.txt") (or use os.CreateTemp("",
suffix+"-*.txt") if testDir is empty), write the test content into the returned
*os.File (or call os.WriteFile on the temp file path), and ensure you close the
file; update the same change for the similar occurrences around lines 78-80
where fileName/filePath and os.WriteFile are used.
- Around line 28-29: The call to os.MkdirAll(testDir, 0755) (and the other
occurrence at lines ~75-77) ignores returned errors; update both sites to
capture the error and fail the test on error (e.g., err := os.MkdirAll(testDir,
0755); if err != nil { t.Fatalf("failed to create %s: %v", testDir, err) } or
use require.NoError(t, err)) so directory setup failures are reported instead of
being swallowed.
- Around line 96-105: The single-shot poll using clie2e.RunCmd with Args
containing "+task_result" and ticket can cause flakes; replace it with a retry
loop that repeatedly calls clie2e.RunCmd(Request{Args: []string{"drive",
"+task_result", "--ticket", ticket, "--scenario", "import"}}) at a short
interval until gjson.Get(pollResult.Stdout, "data.token").String() returns a
non-empty token (docToken) or a timeout is reached; on each attempt assert exit
code and stdout status and break when token is present, and if the timeout
elapses return a clear test failure—implement this around the existing ticket
check and assign docToken only when the token is ready.
In `@tests/cli_e2e/im/helpers_test.go`:
- Around line 63-66: generateSuffix currently uses second precision which leads
to collisions; update the function (generateSuffix) to include higher-resolution
time (e.g., milliseconds or nanoseconds) in the formatted suffix so each call is
unique across rapid/parallel test runs—replace the Format pattern to include
fractional seconds (or use time.Now().UTC().UnixNano() converted to a string)
and ensure callers still expect a string suffix.
---
Nitpick comments:
In `@tests/cli_e2e/im/helpers_test.go`:
- Line 19: The helper API is misleading because createChat registers a no-op
cleanup and sendMessage accepts an unused parentT parameter; update the helpers
by removing the unused parentT parameter from both createChat and sendMessage
and delete the RegisterCleanup no-op call in createChat (or alternatively
implement a real cleanup flow and wire parentT into that cleanup) so the
function signatures and behavior accurately reflect their responsibilities;
locate createChat and sendMessage in the tests/cli_e2e/im/helpers_test.go file
to make these changes.
🪄 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: 471df52c-6c7f-4469-a5aa-8a23061e353a
📒 Files selected for processing (5)
tests/cli_e2e/calendar/calendar_create_event_test.gotests/cli_e2e/docs/docs_create_fetch_test.gotests/cli_e2e/docs/docs_update_test.gotests/cli_e2e/drive/helpers_test.gotests/cli_e2e/im/helpers_test.go
✅ Files skipped from review due to trivial changes (1)
- tests/cli_e2e/docs/docs_create_fetch_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/cli_e2e/docs/docs_update_test.go
- tests/cli_e2e/calendar/calendar_create_event_test.go
tests/cli_e2e/drive/helpers_test.go
Outdated
Comment on lines
+60
to
+64
| parentT.Cleanup(func() { | ||
| clie2e.RunCmd(context.Background(), clie2e.Request{ | ||
| Args: []string{"drive", "files", "delete"}, | ||
| Params: map[string]any{"file_token": fileToken, "type": "file"}, | ||
| }) |
There was a problem hiding this comment.
Make cleanup commands bounded and asserted.
Cleanup currently ignores command errors and uses context.Background() with no timeout, which can hide leaked artifacts or hang teardown.
Suggested fix
parentT.Cleanup(func() {
- clie2e.RunCmd(context.Background(), clie2e.Request{
+ cleanupCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ cleanupRes, cleanupErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{
Args: []string{"drive", "files", "delete"},
Params: map[string]any{"file_token": fileToken, "type": "file"},
})
+ require.NoError(parentT, cleanupErr)
+ cleanupRes.AssertExitCode(parentT, 0)
})
...
parentT.Cleanup(func() {
- clie2e.RunCmd(context.Background(), clie2e.Request{
+ cleanupCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ cleanupRes, cleanupErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{
Args: []string{"drive", "files", "delete"},
Params: map[string]any{"file_token": docToken, "type": "docx"},
})
+ require.NoError(parentT, cleanupErr)
+ cleanupRes.AssertExitCode(parentT, 0)
})Also applies to: 109-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli_e2e/drive/helpers_test.go` around lines 60 - 64, The cleanup
currently calls parentT.Cleanup with an unbounded context and ignores errors
from clie2e.RunCmd; update the cleanup closure used with parentT.Cleanup to
create a context with timeout (e.g., context.WithTimeout) and call clie2e.RunCmd
with that context, capture its returned error and assert/fail if non-nil so
teardown failures are visible; apply the same change to the other cleanup block
around lines 109-113 and reference the clie2e.Request construction and the
clie2e.RunCmd invocation to ensure the deletion command is run with a bounded
context and checked result.
Change-Id: I51e77d857f1fd9aec5ee34adf5045cc695239f21
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/cli_e2e/drive/helpers_test.go (1)
27-28: UsetestFileDirconstant consistently.Both helpers rebuild the same directory path instead of using the shared constant, which increases drift risk.
Also applies to: 84-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/helpers_test.go` around lines 27 - 28, Replace the duplicated construction of the test directory path with the shared testFileDir constant: where the code builds testDir using filepath.Join("tests", "cli_e2e", "drive", "testfiles") and calls os.MkdirAll(testDir, 0o755) (e.g., variable testDir and the filepath.Join/os.MkdirAll usage), use the existing testFileDir constant instead and pass that into os.MkdirAll; do the same fix for the other occurrence around the second helper (the analogous filepath.Join/os.MkdirAll at lines 84-85).
🤖 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/drive/helpers_test.go`:
- Around line 104-107: The test currently only calls result.AssertExitCode(t, 0)
before extracting ticket and docToken from result.Stdout; add a guard call to
result.AssertStdoutStatus(t, 0) (the same envelope-level assertion used at line
59 and 117) immediately after AssertExitCode and before extracting gjson.Get(...
"data.ticket") and gjson.Get(... "data.token") so the test fails early on
envelope errors and safely proceeds to read ticket and docToken.
- Around line 109-126: The polling loop for getting docToken uses RunCmd(ctx,
...) directly so a single RunCmd call can hang and bypass the outer deadline;
change the loop to create a per-attempt context with timeout (e.g.,
context.WithTimeout(parentCtx, smallPerAttemptTimeout or remaining time until
deadline) and pass that ctx to clie2e.RunCmd(Request{...}) for each iteration,
ensuring you call cancel() after each attempt; keep checking the overall
deadline (time.Now().After(deadline)) and break/fail as before if exceeded.
Reference: clie2e.RunCmd, clie2e.Request, ctx, deadline, ticket, docToken in the
loop.
---
Nitpick comments:
In `@tests/cli_e2e/drive/helpers_test.go`:
- Around line 27-28: Replace the duplicated construction of the test directory
path with the shared testFileDir constant: where the code builds testDir using
filepath.Join("tests", "cli_e2e", "drive", "testfiles") and calls
os.MkdirAll(testDir, 0o755) (e.g., variable testDir and the
filepath.Join/os.MkdirAll usage), use the existing testFileDir constant instead
and pass that into os.MkdirAll; do the same fix for the other occurrence around
the second helper (the analogous filepath.Join/os.MkdirAll at lines 84-85).
🪄 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: 0d460d7a-e0be-4206-bb0f-ab07cd155c7d
📒 Files selected for processing (2)
tests/cli_e2e/drive/helpers_test.gotests/cli_e2e/im/helpers_test.go
✅ Files skipped from review due to trivial changes (1)
- tests/cli_e2e/im/helpers_test.go
Change-Id: I3d4f034145bd69fb7640e707fcda05146b8754c7
Change-Id: I40d906c9168754ad71ef9fb770ff4c340fc19beb
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/cli_e2e/drive/helpers_test.go (2)
103-118:⚠️ Potential issue | 🟠 MajorBound each poll attempt with its own timeout.
Line 105 uses
ctxdirectly forRunCmd. If one call blocks, the Line 115 deadline check won’t execute, and polling can hang.Suggested patch
if ticket != "" { deadline := time.Now().Add(45 * time.Second) for { - pollResult, pollErr := clie2e.RunCmd(ctx, clie2e.Request{ + remaining := time.Until(deadline) + if remaining <= 0 { + t.Fatalf("import task did not return token before timeout, ticket=%s", ticket) + } + + attemptTimeout := 10 * time.Second + if remaining < attemptTimeout { + attemptTimeout = remaining + } + + pollCtx, cancel := context.WithTimeout(ctx, attemptTimeout) + pollResult, pollErr := clie2e.RunCmd(pollCtx, clie2e.Request{ Args: []string{"drive", "+task_result", "--ticket", ticket, "--scenario", "import"}, }) + cancel() require.NoError(t, pollErr) pollResult.AssertExitCode(t, 0) pollResult.AssertStdoutStatus(t, true) docToken = gjson.Get(pollResult.Stdout, "data.token").String() if docToken != "" { break } - if time.Now().After(deadline) { - t.Fatalf("import task did not return token before timeout, ticket=%s", ticket) - } time.Sleep(2 * time.Second) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/helpers_test.go` around lines 103 - 118, The polling loop uses the parent ctx directly for clie2e.RunCmd, so a single blocking call can hang the whole loop; wrap each RunCmd invocation with a short per-attempt context timeout (e.g., create ctxAttempt, cancel := context.WithTimeout(ctx, 5*time.Second) before calling clie2e.RunCmd and pass ctxAttempt, then call cancel() after) so each poll attempt times out independently and the outer deadline (deadline variable) can still be checked and enforced; update the RunCmd calls in this loop to use the per-attempt context and handle context.DeadlineExceeded/errors from RunCmd appropriately.
97-100:⚠️ Potential issue | 🟡 MinorAdd envelope status assertion before extracting import fields.
Line 97 checks only exit code. Add
AssertStdoutStatusbefore readingdata.ticket/data.tokenso parse logic is guarded consistently.Suggested patch
require.NoError(t, err) result.AssertExitCode(t, 0) + result.AssertStdoutStatus(t, true) ticket := gjson.Get(result.Stdout, "data.ticket").String() docToken := gjson.Get(result.Stdout, "data.token").String()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/helpers_test.go` around lines 97 - 100, Before extracting import fields, call the test helper to assert the envelope status: add a call to result.AssertStdoutStatus(t, "success") (or the canonical expected status string used in other tests) immediately after result.AssertExitCode(t, 0) and before reading gjson for "data.ticket" and "data.token" so the parse is guarded consistently; locate the block around result.AssertExitCode, result.AssertStdoutStatus, and the gjson.Get(...) lines and insert the AssertStdoutStatus call there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/cli_e2e/drive/helpers_test.go`:
- Around line 103-118: The polling loop uses the parent ctx directly for
clie2e.RunCmd, so a single blocking call can hang the whole loop; wrap each
RunCmd invocation with a short per-attempt context timeout (e.g., create
ctxAttempt, cancel := context.WithTimeout(ctx, 5*time.Second) before calling
clie2e.RunCmd and pass ctxAttempt, then call cancel() after) so each poll
attempt times out independently and the outer deadline (deadline variable) can
still be checked and enforced; update the RunCmd calls in this loop to use the
per-attempt context and handle context.DeadlineExceeded/errors from RunCmd
appropriately.
- Around line 97-100: Before extracting import fields, call the test helper to
assert the envelope status: add a call to result.AssertStdoutStatus(t,
"success") (or the canonical expected status string used in other tests)
immediately after result.AssertExitCode(t, 0) and before reading gjson for
"data.ticket" and "data.token" so the parse is guarded consistently; locate the
block around result.AssertExitCode, result.AssertStdoutStatus, and the
gjson.Get(...) lines and insert the AssertStdoutStatus call there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffeab0f1-f1a9-4fc4-b43c-630024f4838b
📒 Files selected for processing (2)
tests/cli_e2e/drive/helpers_test.gotests/cli_e2e/im/helpers_test.go
✅ Files skipped from review due to trivial changes (1)
- tests/cli_e2e/im/helpers_test.go
Change-Id: I73c21b4b38d4ced7ea27cb327075957ec2b9a2a2
liangshuo-1
previously approved these changes
Apr 10, 2026
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@6923808329ef35b4be5ccd2ff33d2979359dba60🧩 Skill updatenpx skills add yxzhaao/cli#feat/cli-e2e-tests-for-bot -y -g |
yxzhaao
force-pushed
the
feat/cli-e2e-tests-for-bot
branch
from
a8330f9 to
517b59f
Compare
Change-Id: Ied897c37c4f42e446d55d110461aa34ae198195d
yxzhaao
force-pushed
the
feat/cli-e2e-tests-for-bot
branch
from
517b59f to
141d802
Compare
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.
Change-Id: I62edf59d179e407954f65f82e94cf5dcf4938080
Summary
This PR adds a stable subset of CLI E2E tests.
Covered commands:
base:+base-create,+base-get,+table-create,+table-get,+table-list,+advperm-enable,+role-list,+role-get,+role-update,+role-deletecalendar:+event-create,+calendar-create,+calendar-list,+calendar-get,+calendar-update,+agenda,+suggestioncontact:+get-user, plus rawapi get /open-apis/contact/v3/usersfor bot-side user discoverydocs:+docs-create,+docs-get,+docs-updatedrive:files create_folder,files move,permission.members auth,user subscription,user subscription_status,user remove_subscriptionim:+chat-create,+chat-update,chats get,chats link,+messages-send,+messages-mget,+messages-replysheets:+sheet-create,+sheet-get,+sheet-update,+sheet-delete,spreadsheets get,+filter,+findwiki:nodes create,nodes copy,nodes list,spaces get,spaces get_node,spaces listChanges
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit