Add cli e2e tests coverage#400
Closed
yxzhaao wants to merge 18 commits intolarksuite:mainfrom
Closed
Conversation
Change-Id: I43922a6cce5a671e842e48e57e8fb77aae738647
Change-Id: Ic1f8f4fc4a28499c20154b3f7e3a4b5f4ddc46f2
Change-Id: I966c752cec04d52c6af76348b9227ec9e97f4ca4
Change-Id: Id5df27eca7c99c0d0caa46b29b64be30e6f7a29a
Change-Id: I49dc33a0f6639a7ca380373973f0cc2d874e34cd
…/cli into feat/cli-shortcut-e2e-tests Change-Id: Id9b41f5297067ad7fbc00b4d668a7458d5a08058
Change-Id: I6ddaa82e399a8baba277232bdd71a33eea262958
…into feat/e2e-tests
Change-Id: Id66f292991898650a6fa83bd225e83f687e4ee22
…ummary Change-Id: Ie04e887811f34a1f99dd9fb9d08b68db9b3f176a
Change-Id: Iae2774707ea6c7ba11717f0f711f72f8898e09ea
Change-Id: If8dc8da97977f1be869ebfe48cd69350d77694ed
|
zhao.yuxuan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
github-actions
Bot
added
the
size/M
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds extensive CLI end-to-end tests, helpers, and coverage/docs across multiple domains (base, calendar, contact, docs, drive, im, sheets, task, wiki) and a small .gitignore update to ignore Changes
Sequence Diagram(s)(Skipped — changes are test additions and helpers without a new cross-component runtime control flow requiring visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 unit tests (beta)
Comment |
Change-Id: Id5a19567fe24124eb75a56f96b484297b2f99c13
yxzhaao
force-pushed
the
feat/cli-e2e-tests-for-bot-subset
branch
from
0a268fd to
b97e307
Compare
Greptile SummaryThis PR adds comprehensive CLI E2E test coverage across Base, Calendar, Contact, Docs, Drive, IM, Sheets, Task, and Wiki domains, introducing ~30 new test files with well-structured helper utilities and graceful capability skipping. The overall implementation is solid — the base helpers in particular show a mature cleanup pattern — but there is one resource-leak defect in the drive helpers worth addressing before merge.
Confidence Score: 4/5Safe to merge after fixing the drive file-delete cleanup missing type parameter; all other findings are P2. One P1 defect (uploadTestFile cleanup omits required type param, causing silent resource leak on every CI run) warrants a 4 rather than 5. All other findings are cosmetic/style P2. tests/cli_e2e/drive/helpers_test.go (missing type param in cleanup), tests/cli_e2e/im/helpers_test.go (unused helpers + missing EOF newline) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph setup["Test Setup (helpers_test.go)"]
H1[createBase / createTable / createRole]
H2[uploadTestFile / importTestDoc]
H3[createChat / sendMessage]
H4[getPrimaryCalendarID / createEvent]
end
subgraph domains["E2E Test Domains"]
B[Base: basic + role workflows]
CAL[Calendar: event create, manage, agenda, suggestion]
CON[Contact: get-user bot workflow]
DOC[Docs: create+fetch, update]
DRV[Drive: files, move, permission, subscription]
IM[IM: chat, messages]
SH[Sheets: CRUD, filter, find]
TSK[Task: status, reminder, comment, tasklist, my-tasks]
WK[Wiki: node + space workflow]
end
subgraph cleanup["Cleanup (parentT.Cleanup)"]
C1[delete table / field / record / view]
C2[delete drive files - P1: missing type param for regular files]
C3[delete calendar events]
C4[chat: no-op - deletion unavailable]
end
setup --> domains
domains --> cleanup
style C2 fill:#ffcccc,stroke:#cc0000
Reviews (1): Last reviewed commit: "feat: add stable bot-only cli e2e subset" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+71
to
+75
| // The imported document is registered for cleanup via parentT.Cleanup. | ||
| func importTestDoc(t *testing.T, parentT *testing.T, ctx context.Context, suffix, content string) string { | ||
| t.Helper() | ||
|
|
||
| testDir := filepath.Join("tests", "cli_e2e", "drive", "testfiles") |
There was a problem hiding this comment.
"type" parameter in file cleanup
uploadTestFile omits the "type" param from its cleanup call to drive files delete, while both the folder and doc cleanup helpers in the same file explicitly pass it ("type": "folder" and "type": "docx"). If the Lark API requires this parameter for file deletion (it does for folders and docs), the cleanup call will fail silently every time, leaving uploaded test files behind.
Suggested change
| // The imported document is registered for cleanup via parentT.Cleanup. | |
| func importTestDoc(t *testing.T, parentT *testing.T, ctx context.Context, suffix, content string) string { | |
| t.Helper() | |
| testDir := filepath.Join("tests", "cli_e2e", "drive", "testfiles") | |
| parentT.Cleanup(func() { | |
| clie2e.RunCmd(context.Background(), clie2e.Request{ | |
| Args: []string{"drive", "files", "delete"}, | |
| Params: map[string]any{"file_token": fileToken, "type": "file"}, | |
| }) | |
| }) |
Comment on lines
+97
to
+172
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| result.AssertStdoutStatus(t, true) | ||
|
|
||
| messageID := gjson.Get(result.Stdout, "data.message_id").String() | ||
| require.NotEmpty(t, messageID, "message_id should not be empty") | ||
|
|
||
| return messageID | ||
| } | ||
|
|
||
| // sendImage sends an image message to the specified chat and returns the messageID. | ||
| func sendImage(t *testing.T, parentT *testing.T, ctx context.Context, chatID string, imagePath string) string { | ||
| t.Helper() | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{"im", "+messages-send", | ||
| "--chat-id", chatID, | ||
| "--image", imagePath, | ||
| }, | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| result.AssertStdoutStatus(t, true) | ||
|
|
||
| messageID := gjson.Get(result.Stdout, "data.message_id").String() | ||
| require.NotEmpty(t, messageID, "message_id should not be empty") | ||
|
|
||
| return messageID | ||
| } | ||
|
|
||
| // replyMessage sends a reply to a message and returns the reply messageID. | ||
| func replyMessage(t *testing.T, parentT *testing.T, ctx context.Context, messageID string, text string) string { | ||
| t.Helper() | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{"im", "+messages-reply", | ||
| "--message-id", messageID, | ||
| "--text", text, | ||
| }, | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| result.AssertStdoutStatus(t, true) | ||
|
|
||
| replyMessageID := gjson.Get(result.Stdout, "data.message_id").String() | ||
| require.NotEmpty(t, replyMessageID, "reply message_id should not be empty") | ||
|
|
||
| return replyMessageID | ||
| } | ||
|
|
||
| // replyInThread sends a reply in thread to a message and returns the reply messageID. | ||
| func replyInThread(t *testing.T, parentT *testing.T, ctx context.Context, messageID string, text string) string { | ||
| t.Helper() | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{"im", "+messages-reply", | ||
| "--message-id", messageID, | ||
| "--text", text, | ||
| "--reply-in-thread", | ||
| }, | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| result.AssertStdoutStatus(t, true) | ||
|
|
||
| replyMessageID := gjson.Get(result.Stdout, "data.message_id").String() | ||
| require.NotEmpty(t, replyMessageID, "reply message_id should not be empty") | ||
|
|
||
| return replyMessageID | ||
| } | ||
|
|
||
| // generateSuffix generates a unique suffix based on current timestamp. | ||
| func generateSuffix() string { | ||
| return time.Now().UTC().Format("20060102-150405") | ||
| } No newline at end of file |
There was a problem hiding this comment.
sendImage, createChatWithBotManager, and replyInThread are defined in this file but are not called anywhere in the PR. Go does not error on unused functions in test files, but they increase maintenance surface and are a dead-code smell. Either use them in tests or remove them.
Comment on lines
+67
to
+104
|
|
||
| t.Run("create chat with set-bot-manager", func(t *testing.T) { | ||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{"im", "+chat-create", | ||
| "--name", chatName, | ||
| "--type", "private", | ||
| "--set-bot-manager", | ||
| }, | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| result.AssertStdoutStatus(t, true) | ||
|
|
||
| chatID := gjson.Get(result.Stdout, "data.chat_id").String() | ||
| require.NotEmpty(t, chatID, "chat_id should not be empty") | ||
| }) | ||
|
|
||
| t.Run("create public chat with description", func(t *testing.T) { | ||
| publicChatName := chatName + "-public" | ||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{"im", "+chat-create", | ||
| "--name", publicChatName, | ||
| "--type", "public", | ||
| "--description", "Test public chat for e2e", | ||
| }, | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| result.AssertStdoutStatus(t, true) | ||
|
|
||
| publicChatID := gjson.Get(result.Stdout, "data.chat_id").String() | ||
| require.NotEmpty(t, publicChatID) | ||
| }) | ||
| } | ||
|
|
||
| // TestIM_ChatUpdateWorkflow tests the +chat-update shortcut. | ||
| func TestIM_ChatUpdateWorkflow(t *testing.T) { | ||
| parentT := t |
There was a problem hiding this comment.
TestIM_ChatCreateWithOptionsWorkflow
This test creates two chats (one with --set-bot-manager, one public) but neither is cleaned up — the parentT.Cleanup in createChat is a no-op comment noting that deletion is unavailable. However, TestIM_ChatCreateWithOptionsWorkflow doesn't even call createChat; it calls RunCmd directly with no cleanup at all, so these chats will accumulate in the test tenant with every run. Consider registering a best-effort delete if the API exposes it, or at minimum using the shared createChat helper so the no-op cleanup comment is at least consistent.
| // Note: API may have eventual consistency - delete acknowledged but get may still succeed briefly | ||
| _ = result | ||
| }) | ||
| } No newline at end of file |
There was a problem hiding this comment.
calendar_create_event_test.go, calendar/helpers_test.go, calendar_manage_calendar_test.go, drive/drive_move_workflow_test.go, drive/helpers_test.go, docs/docs_create_fetch_test.go, docs/docs_update_test.go, im/helpers_test.go, sheets/sheets_crud_workflow_test.go, and sheets/sheets_filter_workflow_test.go all end without a trailing newline. gofmt (and most lint checks in CI) expect a final newline in every Go source file. Run gofmt -w on these files.
Comment on lines
+165
to
+175
| } | ||
|
|
||
| func createTable(t *testing.T, parentT *testing.T, ctx context.Context, baseToken string, name string, fieldsJSON string, viewJSON string) (tableID string, primaryFieldID string, primaryViewID string) { | ||
| t.Helper() | ||
|
|
||
| args := []string{"base", "+table-create", "--base-token", baseToken, "--name", name} | ||
| if fieldsJSON != "" { | ||
| args = append(args, "--fields", fieldsJSON) | ||
| } | ||
| if viewJSON != "" { | ||
| args = append(args, "--view", viewJSON) |
There was a problem hiding this comment.
testSuffix() (and the equivalent generateSuffix() in the im package) uses time.Now().UTC().Format("20060102-150405"), which has one-second resolution. If two test packages start within the same second, their resource names can collide (e.g., two lark-cli-e2e-base-role-20260410-123456 bases). Adding nanoseconds or a small random component would make suffixes collision-safe:
func testSuffix() string {
return fmt.Sprintf("%s-%04d", time.Now().UTC().Format("20060102-150405"), time.Now().Nanosecond()/100000)
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (12)
.gitignore (1)
37-37: LGTM!Adding
app.logto the ignore list is appropriate for preventing application logs from being committed. The placement under the "# Generated / test artifacts" section is logical.Note: The current pattern
app.logonly ignores this file at the repository root. Ifapp.logfiles might be generated in subdirectories during E2E tests or other operations, consider using a recursive pattern instead.📝 Optional: Make the pattern recursive
If
app.logfiles could appear in subdirectories:-app.log +**/app.logor simply:
-app.log +app.log +**/*/app.logHowever, if the log file is only ever created at the root level, the current pattern is perfectly fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 37, The .gitignore currently has a root-only entry "app.log"; to also ignore app.log files generated in subdirectories (e.g., during E2E tests) add a recursive pattern such as "**/app.log" (or replace "app.log" with "**/app.log") so that the ignore applies to all directories; update the .gitignore entry referencing the existing "app.log" line accordingly.tests/cli_e2e/drive/helpers_test.go (2)
18-19: Unused constanttestFileDir.This constant is defined but never used—the path is duplicated inline at lines 27 and 75. Either remove the constant or use it consistently.
♻️ Suggested fix: use the constant
- testDir := filepath.Join("tests", "cli_e2e", "drive", "testfiles") + testDir := testFileDirApply the same change in
importTestDocat line 75.🤖 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 18 - 19, The constant testFileDir is declared but not used; replace the duplicated inline string literals with this constant: update any occurrences of "tests/cli_e2e/drive/testfiles" (notably inside the function importTestDoc and the other test helper) to use testFileDir instead so the path is centralized; keep the const declaration as-is and ensure importTestDoc and any other helpers reference testFileDir.
27-28: Consider checkingos.MkdirAllerror.Ignoring the error from
os.MkdirAllcould mask CI permission issues or filesystem problems. The same pattern appears at line 76.🛡️ Proposed fix
testDir := filepath.Join("tests", "cli_e2e", "drive", "testfiles") - _ = os.MkdirAll(testDir, 0755) + require.NoError(t, os.MkdirAll(testDir, 0755))🤖 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, The calls to os.MkdirAll (creating testDir) ignore errors and can hide CI/filesystem failures; update both occurrences to check the returned error and fail the test on error (use t.Fatalf or t.Fatal with a message including the error) so the test explicitly handles and reports permission or filesystem issues when creating the test directory referenced by testDir and the similar call later in the file.tests/cli_e2e/base/helpers_test.go (2)
510-522: Consider usingt.TempDir()instead of current working directory.Writing temporary files to the current working directory (
os.Getwd()) can cause issues with parallel test execution and leaves files in potentially shared locations. Usingt.TempDir()provides automatic cleanup and test isolation.♻️ Proposed fix
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 }🤖 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, writeTempAttachment currently creates the temp file in the current working directory which can conflict across tests; change it to use t.TempDir() to create an isolated directory, build the file path with filepath.Join(tempDir, "attachment-"+testSuffix()+".txt"), write the file via os.WriteFile, and remove the manual t.Cleanup removal (t.TempDir handles cleanup) so the function returns "./"+filepath.Base(path) or simply the path as needed by callers; update references to writeTempAttachment if they rely on cwd-relative paths.
491-508: Missing cleanup registration for created workflow.Unlike other
create*helpers in this file,createWorkflowdoesn't take aparentT *testing.Tparameter and doesn't register cleanup. This will leave test workflows behind after test execution.If workflow deletion is supported, consider adding cleanup:
♻️ Proposed fix
-func createWorkflow(t *testing.T, ctx context.Context, baseToken string, body string) string { +func createWorkflow(t *testing.T, parentT *testing.T, ctx context.Context, baseToken string, body string) string { t.Helper() result, err := clie2e.RunCmd(ctx, clie2e.Request{ Args: []string{"base", "+workflow-create", "--base-token", baseToken, "--json", body}, DefaultAs: "bot", }) require.NoError(t, err) if result.ExitCode != 0 { skipIfBaseUnavailable(t, result, "requires bot workflow create capability") } result.AssertExitCode(t, 0) result.AssertStdoutStatus(t, true) workflowID := gjson.Get(result.Stdout, "data.workflow_id").String() require.NotEmpty(t, workflowID, "stdout:\n%s", result.Stdout) + + parentT.Cleanup(func() { + cleanupCtx, cancel := cleanupContext() + defer cancel() + + deleteResult, deleteErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{ + Args: []string{"base", "+workflow-delete", "--base-token", baseToken, "--workflow-id", workflowID, "--yes"}, + DefaultAs: "bot", + }) + if deleteErr != nil || deleteResult.ExitCode != 0 { + reportCleanupFailure(parentT, "delete workflow "+workflowID, deleteResult, deleteErr) + } + }) + return workflowID }🤖 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 491 - 508, createWorkflow currently doesn't register cleanup so created workflows linger; change the helper to accept a parentT *testing.T (or another *testing.T parameter) and register parentT.Cleanup to delete the workflow after the test completes. After obtaining workflowID in createWorkflow, invoke parentT.Cleanup with a function that calls clie2e.RunCmd (similar to the create call) to run "base +workflow-delete --base-token <baseToken> --workflow-id <workflowID>" (and handle non-zero exit like skipIfBaseUnavailable if necessary), ensuring deletion is attempted but does not fail the test if the base is unavailable.tests/cli_e2e/sheets/sheets_filter_workflow_test.go (1)
175-268: Minor:parentTvariable is unused.The
parentTvariable on line 177 is assigned but never used since the cleanup callback is empty. Consider removing it for cleaner code.♻️ Proposed fix
func TestSheets_FindWorkflow(t *testing.T) { - parentT := t ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) t.Cleanup(cancel) ... t.Run("create spreadsheet", func(t *testing.T) { ... - parentT.Cleanup(func() { + t.Cleanup(func() { // Best-effort cleanup })🤖 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 175 - 268, The local variable parentT in TestSheets_FindWorkflow is unused; remove the parentT declaration and the empty parentT.Cleanup callback inside the "create spreadsheet" t.Run block. Edit the TestSheets_FindWorkflow function to delete the line creating parentT and the parentT.Cleanup(...) block so only the existing t and its real cleanup (ctx cancel) remain; no other logic changes to the "create spreadsheet" subtest are required.tests/cli_e2e/wiki/helpers_test.go (1)
57-73: Missing cleanup for created wiki node.The
createWikiNodehelper creates a wiki node but doesn't register a cleanup callback to delete it. Other helper functions in this PR (e.g.,createTable,createField,createRecordin base/helpers_test.go) consistently registerparentT.Cleanupto delete created resources.Consider adding a
parentT *testing.Tparameter and registering cleanup to delete the wiki node, or document why cleanup is intentionally omitted (e.g., if wiki nodes cannot be deleted via CLI).🤖 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 57 - 73, The helper createWikiNode currently returns a created node but never cleans it up; change its signature to accept parentT *testing.T (e.g., createWikiNode(parentT *testing.T, t *testing.T, ctx context.Context, req clie2e.Request) or swap params to match callers), and after obtaining node (from wikiJSONPayload and gjson.Get "data.node") register parentT.Cleanup to run a clie2e.RunCmd delete request that deletes the node (use the node ID from the node result); keep existing skipIfWikiUnavailable and result assertions unchanged so deletion only registers when creation succeeded. Ensure callers are updated to pass parentT and that the cleanup invokes the same CLI path used elsewhere for removing wiki nodes.tests/cli_e2e/calendar/helpers_test.go (1)
39-54: Cleanup callback lacks timeout context.The cleanup callback uses
context.Background()without a timeout, which could cause the test to hang if the delete API is unresponsive. Other helper files (e.g.,base/helpers_test.go) use a dedicatedcleanupContext()with a 30-second timeout.♻️ Proposed fix
+const cleanupTimeout = 30 * time.Second + +func cleanupContext() (context.Context, context.CancelFunc) { + return context.WithTimeout(context.Background(), cleanupTimeout) +} + parentT.Cleanup(func() { - deleteResult, deleteErr := clie2e.RunCmd(context.Background(), clie2e.Request{ + cleanupCtx, cancel := cleanupContext() + defer cancel() + + deleteResult, deleteErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{ Args: []string{"calendar", "events", "delete"},🤖 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 - 54, The cleanup callback uses context.Background() which can hang; change the RunCmd call in the parentT.Cleanup closure to use the timeout context helper (cleanupContext()) instead of context.Background() so the delete request uses the 30s timeout—update the clie2e.RunCmd invocation inside the parentT.Cleanup block (the calendar events delete call that passes calendar_id/event_id) to call cleanupContext() like other helpers (see base/helpers_test.go) so the cleanup will time out reliably.tests/cli_e2e/base/base_basic_workflow_test.go (1)
66-67: Consider adding fallback fortable_idfield.The helper function
createTableinhelpers_test.go(lines 189-192) uses a fallback pattern checking bothdata.table.idanddata.table.table_id. This assertion only checksdata.table.id, which may fail if the API returnstable_idinstead.♻️ Suggested fix to add fallback
- assert.Equal(t, tableID, gjson.Get(result.Stdout, "data.table.id").String()) + returnedTableID := gjson.Get(result.Stdout, "data.table.id").String() + if returnedTableID == "" { + returnedTableID = gjson.Get(result.Stdout, "data.table.table_id").String() + } + assert.Equal(t, tableID, returnedTableID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/base/base_basic_workflow_test.go` around lines 66 - 67, The assertion only checks gjson.Get(result.Stdout, "data.table.id") and should also accept the alternative "data.table.table_id" like the createTable helper does; update the check that compares to tableID to read either gjson.Get(result.Stdout, "data.table.id").String() or gjson.Get(result.Stdout, "data.table.table_id").String() (fall back to the latter when the former is empty) while keeping the tableName assertion unchanged so tests pass regardless of which field the API returns.tests/cli_e2e/im/helpers_test.go (1)
69-86: UnusedparentTparameter.The
parentTparameter is declared but never used insendMessage. The same applies tosendMarkdown,sendImage,replyMessage, andreplyInThread. If these are kept for API consistency or future cleanup, consider adding a brief comment. Otherwise, remove them.♻️ Option 1: Remove unused parameter
-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 {Apply similarly to
sendMarkdown,sendImage,replyMessage, andreplyInThread.♻️ Option 2: Add comment explaining the signature
// sendMessage sends a text message to the specified chat and returns the messageID. +// parentT is accepted for API consistency with other helpers but is unused since message cleanup is not implemented. func sendMessage(t *testing.T, parentT *testing.T, ctx context.Context, chatID string, text string) string { + _ = parentT // unused: message cleanup not available via CLI🤖 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 69 - 86, The helper functions declare an unused parentT parameter—remove the unused parentT from the signatures of sendMessage, sendMarkdown, sendImage, replyMessage, and replyInThread and update all call sites to drop that argument; ensure you only change the function signatures and their invocations (e.g., calls to sendMessage(ctx, chatID, text) instead of sendMessage(parentT, ctx, chatID, text)) so tests compile and no unused parameter warnings remain.tests/cli_e2e/calendar/calendar_create_event_test.go (1)
94-107: The "verify delete acknowledged" step doesn't verify anything.The test runs the
getcommand but discards the result with_ = result. If the intent is to verify eventual consistency, consider either:
- Removing this step if it adds no value
- Adding a brief comment explaining why the result is intentionally ignored
- Actually checking for an expected error status indicating the event is deleted
♻️ Option: Remove or clarify the step
- // 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 - })Or if keeping it, make the intent clearer:
t.Run("verify delete acknowledged", func(t *testing.T) { require.NotEmpty(t, eventID) - result, err := clie2e.RunCmd(ctx, clie2e.Request{ + // Best-effort check: command execution should succeed even if event is already deleted + // We don't assert on the response due to eventual consistency + _, 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 })🤖 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 94 - 107, The "verify delete acknowledged" subtest currently calls clie2e.RunCmd in tests/cli_e2e/calendar/calendar_create_event_test.go and then discards the response (result) which means it verifies nothing; update the subtest (named "verify delete acknowledged") to either remove the test entirely or assert a meaningful outcome: for example, call clie2e.RunCmd with Args {"calendar","events","get"} and Params {"calendar_id": calendarID, "event_id": eventID} and then assert the expected deletion behavior (e.g., require.Error(t, err) and check the error indicates NotFound or require.Contains(t, result.StdErr, "not found") if the CLI returns non-zero output), or add a short comment explaining that the response is intentionally ignored for eventual consistency — reference the variables eventID, calendarID and the clie2e.RunCmd invocation when making the change.tests/cli_e2e/im/chat_workflow_test.go (1)
115-137: Verify persisted state after chat updates, not only success envelopes.Both update subtests assert only exit/status. Add a follow-up
im chats getassertion forname/descriptionto actually prove mutation persisted.🤖 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, The tests "update chat name" and "update chat description" only assert success envelopes; after calling clie2e.RunCmd with Args ["im", "+chat-update", "--chat-id", chatID, "--name", updatedName] and similarly for description, add a follow-up clie2e.RunCmd invocation that runs ["im", "chats", "get", "--chat-id", chatID] and assert the returned stdout contains the updated value (updatedName or updatedDescription) and still exits 0; update the subtests to call RunCmd for "im chats get", parse/inspect stdout, and use require/assert helpers to validate the persisted field so the mutation is actually verified.
🤖 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/base/base_role_workflow_test.go`:
- Line 35: The test creates a role via createRole(...) which already registers a
parentT.Cleanup delete (see parentT.Cleanup in helpers_test.go), so avoid
double-deleting by either removing the explicit "+role-delete" subtest or
changing the create call to a non-cleanup variant; locate the role creation line
where roleID := createRole(t, parentT, ctx, baseToken,
`{"role_name":"`+roleName+`","role_type":"custom_role"}`) and either (A) delete
the subtest that calls the workflow "+role-delete" later in this test, or (B)
replace createRole with a helper that does not register parentT.Cleanup so the
explicit delete remains — apply the same change for the other occurrence around
lines 118-129.
- Around line 17-20: The test TestBase_RoleWorkflow currently doesn't isolate
CLI config and can leak state; update the test setup to set a unique config
directory by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) early in
TestBase_RoleWorkflow (before any CLI operations or goroutines) so each run uses
an isolated config dir; ensure this is added alongside the existing
context/cancel setup using the test's t variable.
In `@tests/cli_e2e/calendar/calendar_find_meeting_time_test.go`:
- Around line 16-19: In TestCalendar_FindMeetingTime, the CLI config state isn't
isolated and can leak between tests; set a test-local config dir by calling
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) near the start of the test
(after ctx/cancel creation) so all CLI invocations in
TestCalendar_FindMeetingTime use an isolated config directory.
In `@tests/cli_e2e/calendar/calendar_view_agenda_test.go`:
- Around line 16-18: In TestCalendar_ViewAgenda, isolate per-test config state
by setting the LARKSUITE_CLI_CONFIG_DIR to a temp directory before any CLI
invocation: call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start
of the TestCalendar_ViewAgenda function (after ctx creation is fine) so the test
reads/writes only test-local config and avoids shared state in parallel/CI runs.
In `@tests/cli_e2e/cli-e2e-testcase-writer/SKILL.md`:
- Line 107: Update the phrasing to use hyphenation for readability: replace the
phrase "go test -run friendly form" with "`go test -run`-friendly form" in the
SKILL.md entry (the line that currently reads "Write testcase entries in `go
test -run` friendly form.").
In `@tests/cli_e2e/docs/docs_create_fetch_test.go`:
- Around line 43-45: The empty parentT.Cleanup is leaving test-created documents
persisted; track created document IDs in the test (e.g., a slice named
createdDocIDs populated when calling the create routine) and implement a
best-effort teardown inside parentT.Cleanup that iterates createdDocIDs and
calls the document deletion API (e.g., docsClient.DeleteDocument or
docsClient.Delete(ctx, docID)) for each ID, ignoring or logging errors but not
failing the test; ensure you reference the same client/ctx variables used
elsewhere in the test so Cleanup can delete the remote docs it created.
- Around line 18-22: The test TestDocs_CreateAndFetchWorkflow does not isolate
CLI config and may leak auth/config between runs; fix by setting the environment
variable LARKSUITE_CLI_CONFIG_DIR to a per-test temp dir at the start of
TestDocs_CreateAndFetchWorkflow (use t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir())) so the test uses an isolated config directory for its lifetime.
In `@tests/cli_e2e/docs/docs_update_test.go`:
- Around line 45-47: Replace the no-op parentT.Cleanup stub with a best-effort
teardown that removes or archives any docs created by the test: inside the
parentT.Cleanup(func() { ... }) body iterate over the test's created-doc
identifiers (e.g., createdDocIDs or similar test-scoped slice), call the
appropriate deletion/archive helper on your client (e.g., client.DeleteDoc or
client.ArchiveDoc/TeardownDocs) for each ID, swallow/log but do not return
errors (best-effort), and ensure any client or context variables used in tests
are captured/closed; if teardown is intentionally deferred, remove the stub
instead.
- Around line 18-22: TestDocs_UpdateWorkflow shares CLI config state causing
flakiness; modify the test (TestDocs_UpdateWorkflow) to isolate config by
calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) early in the test
setup (e.g., immediately after parentT := t or right after ctx creation) so the
CLI uses a unique temporary config directory for this test only.
In `@tests/cli_e2e/im/chat_workflow_test.go`:
- Line 157: Remove or redact raw chat payloads and share links being printed in
CI logs: stop calling t.Logf with result.Stdout in chat_workflow_test.go (and
the similar log at line 194) and instead either remove the log entirely or log a
sanitized summary (e.g., status/length or a redacted string). Locate the test
helper or variable names result and share_link in the test (e.g., the
t.Logf("chats get result: %s", result.Stdout) call) and replace it with a
non-sensitive assertion or a redacted/summarized log entry that strips
identifiers and URLs before printing.
- Around line 17-20: In TestIM_ChatCreateSendWorkflow add isolation for CLI
config state by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) near
the top (e.g., immediately after parentT := t) so each top-level e2e test uses
its own config dir; apply the same change to the other top-level test functions
in this file (the ones that start other workflows) to prevent auth/config state
bleeding between runs.
In `@tests/cli_e2e/README.md`:
- Line 115: Replace the hardcoded absolute path in the PLAYWRIGHT_STORAGE_STATE
export with a relative path or placeholder; update the export line that sets
PLAYWRIGHT_STORAGE_STATE so it points to a repo-relative location (e.g., a
./tests/... path) or a placeholder like ${PLAYWRIGHT_STORAGE_STATE_PATH} so
other contributors can set their own value, and document the expected default in
the README.
---
Nitpick comments:
In @.gitignore:
- Line 37: The .gitignore currently has a root-only entry "app.log"; to also
ignore app.log files generated in subdirectories (e.g., during E2E tests) add a
recursive pattern such as "**/app.log" (or replace "app.log" with "**/app.log")
so that the ignore applies to all directories; update the .gitignore entry
referencing the existing "app.log" line accordingly.
In `@tests/cli_e2e/base/base_basic_workflow_test.go`:
- Around line 66-67: The assertion only checks gjson.Get(result.Stdout,
"data.table.id") and should also accept the alternative "data.table.table_id"
like the createTable helper does; update the check that compares to tableID to
read either gjson.Get(result.Stdout, "data.table.id").String() or
gjson.Get(result.Stdout, "data.table.table_id").String() (fall back to the
latter when the former is empty) while keeping the tableName assertion unchanged
so tests pass regardless of which field the API returns.
In `@tests/cli_e2e/base/helpers_test.go`:
- Around line 510-522: writeTempAttachment currently creates the temp file in
the current working directory which can conflict across tests; change it to use
t.TempDir() to create an isolated directory, build the file path with
filepath.Join(tempDir, "attachment-"+testSuffix()+".txt"), write the file via
os.WriteFile, and remove the manual t.Cleanup removal (t.TempDir handles
cleanup) so the function returns "./"+filepath.Base(path) or simply the path as
needed by callers; update references to writeTempAttachment if they rely on
cwd-relative paths.
- Around line 491-508: createWorkflow currently doesn't register cleanup so
created workflows linger; change the helper to accept a parentT *testing.T (or
another *testing.T parameter) and register parentT.Cleanup to delete the
workflow after the test completes. After obtaining workflowID in createWorkflow,
invoke parentT.Cleanup with a function that calls clie2e.RunCmd (similar to the
create call) to run "base +workflow-delete --base-token <baseToken>
--workflow-id <workflowID>" (and handle non-zero exit like skipIfBaseUnavailable
if necessary), ensuring deletion is attempted but does not fail the test if the
base is unavailable.
In `@tests/cli_e2e/calendar/calendar_create_event_test.go`:
- Around line 94-107: The "verify delete acknowledged" subtest currently calls
clie2e.RunCmd in tests/cli_e2e/calendar/calendar_create_event_test.go and then
discards the response (result) which means it verifies nothing; update the
subtest (named "verify delete acknowledged") to either remove the test entirely
or assert a meaningful outcome: for example, call clie2e.RunCmd with Args
{"calendar","events","get"} and Params {"calendar_id": calendarID, "event_id":
eventID} and then assert the expected deletion behavior (e.g., require.Error(t,
err) and check the error indicates NotFound or require.Contains(t,
result.StdErr, "not found") if the CLI returns non-zero output), or add a short
comment explaining that the response is intentionally ignored for eventual
consistency — reference the variables eventID, calendarID and the clie2e.RunCmd
invocation when making the change.
In `@tests/cli_e2e/calendar/helpers_test.go`:
- Around line 39-54: The cleanup callback uses context.Background() which can
hang; change the RunCmd call in the parentT.Cleanup closure to use the timeout
context helper (cleanupContext()) instead of context.Background() so the delete
request uses the 30s timeout—update the clie2e.RunCmd invocation inside the
parentT.Cleanup block (the calendar events delete call that passes
calendar_id/event_id) to call cleanupContext() like other helpers (see
base/helpers_test.go) so the cleanup will time out reliably.
In `@tests/cli_e2e/drive/helpers_test.go`:
- Around line 18-19: The constant testFileDir is declared but not used; replace
the duplicated inline string literals with this constant: update any occurrences
of "tests/cli_e2e/drive/testfiles" (notably inside the function importTestDoc
and the other test helper) to use testFileDir instead so the path is
centralized; keep the const declaration as-is and ensure importTestDoc and any
other helpers reference testFileDir.
- Around line 27-28: The calls to os.MkdirAll (creating testDir) ignore errors
and can hide CI/filesystem failures; update both occurrences to check the
returned error and fail the test on error (use t.Fatalf or t.Fatal with a
message including the error) so the test explicitly handles and reports
permission or filesystem issues when creating the test directory referenced by
testDir and the similar call later in the file.
In `@tests/cli_e2e/im/chat_workflow_test.go`:
- Around line 115-137: The tests "update chat name" and "update chat
description" only assert success envelopes; after calling clie2e.RunCmd with
Args ["im", "+chat-update", "--chat-id", chatID, "--name", updatedName] and
similarly for description, add a follow-up clie2e.RunCmd invocation that runs
["im", "chats", "get", "--chat-id", chatID] and assert the returned stdout
contains the updated value (updatedName or updatedDescription) and still exits
0; update the subtests to call RunCmd for "im chats get", parse/inspect stdout,
and use require/assert helpers to validate the persisted field so the mutation
is actually verified.
In `@tests/cli_e2e/im/helpers_test.go`:
- Around line 69-86: The helper functions declare an unused parentT
parameter—remove the unused parentT from the signatures of sendMessage,
sendMarkdown, sendImage, replyMessage, and replyInThread and update all call
sites to drop that argument; ensure you only change the function signatures and
their invocations (e.g., calls to sendMessage(ctx, chatID, text) instead of
sendMessage(parentT, ctx, chatID, text)) so tests compile and no unused
parameter warnings remain.
In `@tests/cli_e2e/sheets/sheets_filter_workflow_test.go`:
- Around line 175-268: The local variable parentT in TestSheets_FindWorkflow is
unused; remove the parentT declaration and the empty parentT.Cleanup callback
inside the "create spreadsheet" t.Run block. Edit the TestSheets_FindWorkflow
function to delete the line creating parentT and the parentT.Cleanup(...) block
so only the existing t and its real cleanup (ctx cancel) remain; no other logic
changes to the "create spreadsheet" subtest are required.
In `@tests/cli_e2e/wiki/helpers_test.go`:
- Around line 57-73: The helper createWikiNode currently returns a created node
but never cleans it up; change its signature to accept parentT *testing.T (e.g.,
createWikiNode(parentT *testing.T, t *testing.T, ctx context.Context, req
clie2e.Request) or swap params to match callers), and after obtaining node (from
wikiJSONPayload and gjson.Get "data.node") register parentT.Cleanup to run a
clie2e.RunCmd delete request that deletes the node (use the node ID from the
node result); keep existing skipIfWikiUnavailable and result assertions
unchanged so deletion only registers when creation succeeded. Ensure callers are
updated to pass parentT and that the cleanup invokes the same CLI path used
elsewhere for removing wiki nodes.
🪄 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: 80af146c-8b4e-43ee-b4c1-21da61f681f2
📒 Files selected for processing (34)
.gitignoretests/cli_e2e/README.mdtests/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/cli-e2e-testcase-writer/SKILL.mdtests/cli_e2e/contact/contact_shortcut_test.gotests/cli_e2e/coverage.mdtests/cli_e2e/demo/coverage.mdtests/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/task/coverage.mdtests/cli_e2e/task/task_comment_workflow_test.gotests/cli_e2e/task/task_get_my_tasks_user_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.gotests/cli_e2e/wiki/helpers_test.gotests/cli_e2e/wiki/wiki_workflow_test.go
Comment on lines
+17
to
+20
| func TestBase_RoleWorkflow(t *testing.T) { | ||
| parentT := t | ||
| ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute) | ||
| t.Cleanup(cancel) |
There was a problem hiding this comment.
Isolate CLI config per test run.
This test does not isolate LARKSUITE_CLI_CONFIG_DIR, so state can leak across parallel/previous runs and make e2e results flaky.
As per coding guidelines **/*_test.go: “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/base/base_role_workflow_test.go` around lines 17 - 20, The test
TestBase_RoleWorkflow currently doesn't isolate CLI config and can leak state;
update the test setup to set a unique config directory by calling
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) early in TestBase_RoleWorkflow
(before any CLI operations or goroutines) so each run uses an isolated config
dir; ensure this is added alongside the existing context/cancel setup using the
test's t variable.
| result.AssertStdoutStatus(t, true) | ||
|
|
||
| roleName := "Reviewer-" + testSuffix() | ||
| roleID := createRole(t, parentT, ctx, baseToken, `{"role_name":"`+roleName+`","role_type":"custom_role"}`) |
There was a problem hiding this comment.
Avoid deleting the same role twice (workflow delete + helper cleanup).
createRole(...) already registers a parentT.Cleanup delete in tests/cli_e2e/base/helpers_test.go:428-489. Running +role-delete again in this test can cause cleanup failure/noise when cleanup executes.
Consider either:
- keep the explicit
deletesubtest and create the role without auto-cleanup helper, or - keep helper cleanup and drop the explicit delete subtest.
Also applies to: 118-129
🤖 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` at line 35, The test creates a
role via createRole(...) which already registers a parentT.Cleanup delete (see
parentT.Cleanup in helpers_test.go), so avoid double-deleting by either removing
the explicit "+role-delete" subtest or changing the create call to a non-cleanup
variant; locate the role creation line where roleID := createRole(t, parentT,
ctx, baseToken, `{"role_name":"`+roleName+`","role_type":"custom_role"}`) and
either (A) delete the subtest that calls the workflow "+role-delete" later in
this test, or (B) replace createRole with a helper that does not register
parentT.Cleanup so the explicit delete remains — apply the same change for the
other occurrence around lines 118-129.
Comment on lines
+16
to
+19
| func TestCalendar_FindMeetingTime(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | ||
| t.Cleanup(cancel) | ||
|
|
There was a problem hiding this comment.
Isolate CLI config state per test to avoid cross-test contamination.
This test invokes CLI commands without setting a test-local config dir, which can leak shared state across runs.
🔧 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)As per coding guidelines, **/*_test.go: 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/calendar/calendar_find_meeting_time_test.go` around lines 16 -
19, In TestCalendar_FindMeetingTime, the CLI config state isn't isolated and can
leak between tests; set a test-local config dir by calling
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) near the start of the test
(after ctx/cancel creation) so all CLI invocations in
TestCalendar_FindMeetingTime use an isolated config directory.
Comment on lines
+16
to
+18
| func TestCalendar_ViewAgenda(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | ||
| t.Cleanup(cancel) |
There was a problem hiding this comment.
Add per-test config directory isolation before running CLI commands.
Without a test-local config dir, this test may read/write shared user state and become flaky in parallel or CI runs.
🔧 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)As per coding guidelines, **/*_test.go: Isolate config state in Go tests by using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestCalendar_ViewAgenda(t *testing.T) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | |
| t.Cleanup(cancel) | |
| 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 - 18, In
TestCalendar_ViewAgenda, isolate per-test config state by setting the
LARKSUITE_CLI_CONFIG_DIR to a temp directory before any CLI invocation: call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of the
TestCalendar_ViewAgenda function (after ctx creation is fine) so the test
reads/writes only test-local config and avoids shared state in parallel/CI runs.
|
|
||
| Then assert the business fields with `gjson`. | ||
| - Mark each command `shortcut` or `api`. | ||
| - Write testcase entries in `go test -run` friendly form. |
There was a problem hiding this comment.
Use hyphenated phrasing for readability.
Change “go test -run friendly form” to “go test -run-friendly form”.
🧰 Tools
🪛 LanguageTool
[grammar] ~107-~107: Use a hyphen to join words.
Context: ...Write testcase entries in go test -run friendly form. - Commands only exercised...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli_e2e/cli-e2e-testcase-writer/SKILL.md` at line 107, Update the
phrasing to use hyphenation for readability: replace the phrase "go test -run
friendly form" with "`go test -run`-friendly form" in the SKILL.md entry (the
line that currently reads "Write testcase entries in `go test -run` friendly
form.").
Comment on lines
+18
to
+22
| func TestDocs_UpdateWorkflow(t *testing.T) { | ||
| parentT := t | ||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | ||
| t.Cleanup(cancel) | ||
|
|
There was a problem hiding this comment.
Isolate CLI config state for this test.
This test currently shares config state with other tests/processes, which can make e2e runs flaky.
🔧 Proposed 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 using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestDocs_UpdateWorkflow(t *testing.T) { | |
| parentT := t | |
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | |
| t.Cleanup(cancel) | |
| 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) | |
🤖 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,
TestDocs_UpdateWorkflow shares CLI config state causing flakiness; modify the
test (TestDocs_UpdateWorkflow) to isolate config by calling
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) early in the test setup (e.g.,
immediately after parentT := t or right after ctx creation) so the CLI uses a
unique temporary config directory for this test only.
Comment on lines
+45
to
+47
| parentT.Cleanup(func() { | ||
| // best-effort cleanup | ||
| }) |
There was a problem hiding this comment.
Replace the no-op cleanup with real teardown.
The cleanup registered on Line 45 does nothing, so created docs are left behind after test runs. Please implement actual best-effort deletion/archive there (or remove the stub if teardown is intentionally deferred).
🤖 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 45 - 47, Replace the
no-op parentT.Cleanup stub with a best-effort teardown that removes or archives
any docs created by the test: inside the parentT.Cleanup(func() { ... }) body
iterate over the test's created-doc identifiers (e.g., createdDocIDs or similar
test-scoped slice), call the appropriate deletion/archive helper on your client
(e.g., client.DeleteDoc or client.ArchiveDoc/TeardownDocs) for each ID,
swallow/log but do not return errors (best-effort), and ensure any client or
context variables used in tests are captured/closed; if teardown is
intentionally deferred, remove the stub instead.
Comment on lines
+17
to
+20
| func TestIM_ChatCreateSendWorkflow(t *testing.T) { | ||
| parentT := t | ||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | ||
| t.Cleanup(cancel) |
There was a problem hiding this comment.
Set an isolated config dir in each top-level e2e test.
These tests don’t isolate LARKSUITE_CLI_CONFIG_DIR, so CLI auth/config state can bleed between runs.
As per coding guidelines **/*_test.go: “Isolate config state in Go tests by using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())”.
Also applies to: 62-63, 105-107, 143-145, 172-174
🤖 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 - 20, In
TestIM_ChatCreateSendWorkflow add isolation for CLI config state by calling
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) near the top (e.g.,
immediately after parentT := t) so each top-level e2e test uses its own config
dir; apply the same change to the other top-level test functions in this file
(the ones that start other workflows) to prevent auth/config state bleeding
between runs.
| Then run E2E with: | ||
|
|
||
| ```bash | ||
| export PLAYWRIGHT_STORAGE_STATE=/Users/bytedance/cli/tests/cli_e2e/browser/.auth/state.json |
There was a problem hiding this comment.
Replace hardcoded absolute path with a relative or placeholder path.
The path /Users/bytedance/cli/tests/cli_e2e/browser/.auth/state.json is user-specific and won't work for other contributors. Consider using a relative path or a placeholder.
📝 Suggested fix
-export PLAYWRIGHT_STORAGE_STATE=/Users/bytedance/cli/tests/cli_e2e/browser/.auth/state.json
+export PLAYWRIGHT_STORAGE_STATE=$(pwd)/tests/cli_e2e/browser/.auth/state.jsonOr use a placeholder:
-export PLAYWRIGHT_STORAGE_STATE=/Users/bytedance/cli/tests/cli_e2e/browser/.auth/state.json
+export PLAYWRIGHT_STORAGE_STATE=/path/to/cli/tests/cli_e2e/browser/.auth/state.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export PLAYWRIGHT_STORAGE_STATE=/Users/bytedance/cli/tests/cli_e2e/browser/.auth/state.json | |
| export PLAYWRIGHT_STORAGE_STATE=$(pwd)/tests/cli_e2e/browser/.auth/state.json |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli_e2e/README.md` at line 115, Replace the hardcoded absolute path in
the PLAYWRIGHT_STORAGE_STATE export with a relative path or placeholder; update
the export line that sets PLAYWRIGHT_STORAGE_STATE so it points to a
repo-relative location (e.g., a ./tests/... path) or a placeholder like
${PLAYWRIGHT_STORAGE_STATE_PATH} so other contributors can set their own value,
and document the expected default in the README.
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
5 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
This PR adds a stable set 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 listSummary by CodeRabbit
Tests
Documentation
Chores