Closed
Conversation
Change-Id: I43922a6cce5a671e842e48e57e8fb77aae738647
github-actions
Bot
added
the
size/S
|
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. |
yxzhaao
force-pushed
the
feat/cli-shortcut-e2e-tests
branch
from
54933f4 to
dccdb71
Compare
|
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:
📝 WalkthroughWalkthroughThis PR adds comprehensive CLI E2E test coverage across multiple domains (base, wiki, task) with reusable helper utilities for JSON payload extraction, capability-aware test skipping, and resource lifecycle management. It includes new workflow test files exercising CLI command sequences, along with coverage documentation for each domain. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Greptile SummaryThis PR adds a large suite of E2E tests across Wiki, Base, Task, IM, Drive, Docs, Sheets, Calendar, and Contact domains (65 files, ~6,600 lines). The tests follow a consistent pattern of setup helpers, skip-on-unavailable guards, and cleanup callbacks — the Base and IM packages in particular have well-structured helper layers.
Confidence Score: 4/5Safe to merge after fixing the broken reaction delete subtest; all other tests follow correct patterns. One P1 finding: the reaction delete subtest is a complete no-op due to a hardcoded invalid ID and absent assertions, meaning the delete code path is never verified. Everything else — Base, Task, Drive, Docs, Wiki, Calendar, Sheets, and the other IM tests — looks correct and well-structured. tests/cli_e2e/im/reaction_workflow_test.go — the delete reaction subtest needs the reaction ID wired through from function scope.
|
| Filename | Overview |
|---|---|
| tests/cli_e2e/im/reaction_workflow_test.go | Adds reaction list/create/delete workflow tests, but the delete subtest hardcodes "invalid_reaction_id" and omits all assertions, making it a no-op. |
| tests/cli_e2e/base/helpers_test.go | Adds a large set of shared helpers (createBase, createTable, createRecord, etc.) with robust cleanup handling and skip-on-unavailable logic; no issues found. |
| tests/cli_e2e/base/base_table_record_view_workflow_test.go | Comprehensive table/field/record/view workflow test (454 lines); uses consistent true for shortcut commands and cleanup callbacks correctly. |
| tests/cli_e2e/task/tasklist_workflow_test.go | Adds tasklist create/get/list-tasks/get-task workflow with correct cleanup registration inside the create subtest. |
| tests/cli_e2e/base/base_role_workflow_test.go | Adds role create/list/get/update/delete workflow test that correctly gates on advperm being enabled first; no issues found. |
| tests/cli_e2e/im/message_workflow_test.go | Adds message send/update/delete/forward workflow tests; some forward/merge-forward subtests use obviously invalid IDs with no assertions, functioning as skeleton tests only. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[TestIM_ReactionsWorkflow] --> B[createChat]
B --> C[sendMessage]
C --> D["list reactions\n(AssertStdoutStatus 0)"]
D --> E["create reaction SMILE\n(reactionID stored locally only)"]
E --> F["batch_query reactions\n(AssertStdoutStatus 0)"]
F --> G["delete reaction\n(BROKEN: uses 'invalid_reaction_id'\nno assertions)"]
G:::broken
classDef broken fill:#ff6b6b,color:#fff,stroke:#cc0000
Reviews (13): Last reviewed commit: "docs: update docs and contact e2e covera..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/cli_e2e/mail/helpers_test.go (2)
23-31: JSON extraction assumes payload extends to end of string.The logic finds the start of JSON via
\n{or{, then takesraw[start:]to the end. If there's trailing non-JSON content after the closing},gjson.Validwould fail. This works for current CLI output but is fragile.For robustness, consider matching balanced braces or documenting that CLI output must end with the JSON payload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/mail/helpers_test.go` around lines 23 - 31, The test currently takes payload := raw[start:] which assumes JSON runs to EOF; change this to locate the end of the JSON by scanning from start to the matching closing brace and slice raw[start:end+1] instead. Implement a small brace-matching scan that tracks depth, in-string state and escape chars (so braces inside strings are ignored) to find the correct end index, then set payload to that substring and use it for gjson.Valid checks; reference the existing variables start, raw and payload in helpers_test.go when making the change.
18-21: Fallback fromStdouttoStderrmay extract error payloads on failed commands.Per
tests/cli_e2e/core.go:48-95,Resultpopulates bothStdoutandStderrregardless of exit code. When a command fails (exit code ≠ 0), error JSON typically goes toStderrwhileStdoutmay be empty. This fallback would then extract the error payload—which may be intentional for tests likeTestMail_TriagePermissionConstraint_Bot, but could mask issues if a test incorrectly expects success output.Consider documenting this behavior or adding an explicit parameter to control which stream to parse, so callers can be explicit about their intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/mail/helpers_test.go` around lines 18 - 21, The helper currently falls back from result.Stdout to result.Stderr by assigning raw := strings.TrimSpace(result.Stdout) then using stderr when stdout is empty, which can unintentionally parse error payloads; modify the helper to accept an explicit flag (e.g., parseFromStderr bool or preferStderr bool) or an enum (StdoutOnly/AllowStderr) and use that to choose between result.Stdout and result.Stderr instead of implicit fallback, update the call sites (e.g., tests that expect error JSON like TestMail_TriagePermissionConstraint_Bot) to pass the appropriate flag, and add a brief comment documenting the semantics of the new parameter so callers are explicit about which stream to parse.tests/cli_e2e/mail/mail_user_reference_test.go (1)
21-32: Consider addingAssertStdoutStatusfor consistency with the other subtest.The "watch print output schema" subtest calls
AssertExitCodebut notAssertStdoutStatus, while the "draft edit print patch template" subtest at line 41 calls both. For consistency across subtests, consider adding the status assertion here as well.Proposed fix
require.NoError(t, err) result.AssertExitCode(t, 0) + result.AssertStdoutStatus(t, true) payload := mailJSONPayload(t, result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/mail/mail_user_reference_test.go` around lines 21 - 32, The "watch print output schema" subtest calls result.AssertExitCode but is missing the stdout status check for consistency; update the subtest (inside t.Run "watch print output schema") to call result.AssertStdoutStatus(t, "ok") after require.NoError and before parsing payload so it mirrors the other subtest's use of AssertStdoutStatus on the same result object returned by clie2e.RunCmd.tests/cli_e2e/wiki/wiki_workflow_test.go (1)
21-23: Strengthen title uniqueness for parallel CI runs (Line 21).Second-level timestamps can collide; include sub-second entropy.
🔧 Suggested patch
- suffix := time.Now().UTC().Format("20060102-150405") + suffix := time.Now().UTC().Format("20060102-150405.000000000")🤖 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 21 - 23, The test uses a second-granularity suffix for createdTitle and copiedTitle which can collide in parallel CI; update the suffix generation (variable suffix) to include sub-second entropy (e.g., append milliseconds or nanoseconds via time.Now().UTC().Format with .000 or .000000000 or use time.Now().UTC().UnixNano()) so createdTitle and copiedTitle become globally unique across fast concurrent runs; change the suffix variable and keep the same createdTitle and copiedTitle identifiers.
🤖 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/wiki/wiki_workflow_test.go`:
- Around line 17-20: TestWiki_Workflow_Bot currently uses shared CLI config and
can leak state between tests; at the start of the TestWiki_Workflow_Bot function
add config isolation by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) so the test uses a unique temporary config directory, ensuring
cleanup via t.TempDir(); place this call immediately after ctx/cancel setup in
TestWiki_Workflow_Bot to isolate CLI config state for the test.
---
Nitpick comments:
In `@tests/cli_e2e/mail/helpers_test.go`:
- Around line 23-31: The test currently takes payload := raw[start:] which
assumes JSON runs to EOF; change this to locate the end of the JSON by scanning
from start to the matching closing brace and slice raw[start:end+1] instead.
Implement a small brace-matching scan that tracks depth, in-string state and
escape chars (so braces inside strings are ignored) to find the correct end
index, then set payload to that substring and use it for gjson.Valid checks;
reference the existing variables start, raw and payload in helpers_test.go when
making the change.
- Around line 18-21: The helper currently falls back from result.Stdout to
result.Stderr by assigning raw := strings.TrimSpace(result.Stdout) then using
stderr when stdout is empty, which can unintentionally parse error payloads;
modify the helper to accept an explicit flag (e.g., parseFromStderr bool or
preferStderr bool) or an enum (StdoutOnly/AllowStderr) and use that to choose
between result.Stdout and result.Stderr instead of implicit fallback, update the
call sites (e.g., tests that expect error JSON like
TestMail_TriagePermissionConstraint_Bot) to pass the appropriate flag, and add a
brief comment documenting the semantics of the new parameter so callers are
explicit about which stream to parse.
In `@tests/cli_e2e/mail/mail_user_reference_test.go`:
- Around line 21-32: The "watch print output schema" subtest calls
result.AssertExitCode but is missing the stdout status check for consistency;
update the subtest (inside t.Run "watch print output schema") to call
result.AssertStdoutStatus(t, "ok") after require.NoError and before parsing
payload so it mirrors the other subtest's use of AssertStdoutStatus on the same
result object returned by clie2e.RunCmd.
In `@tests/cli_e2e/wiki/wiki_workflow_test.go`:
- Around line 21-23: The test uses a second-granularity suffix for createdTitle
and copiedTitle which can collide in parallel CI; update the suffix generation
(variable suffix) to include sub-second entropy (e.g., append milliseconds or
nanoseconds via time.Now().UTC().Format with .000 or .000000000 or use
time.Now().UTC().UnixNano()) so createdTitle and copiedTitle become globally
unique across fast concurrent runs; change the suffix variable and keep the same
createdTitle and copiedTitle identifiers.
🪄 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: 0912aad9-a823-45c2-8200-f7495a9654e2
📒 Files selected for processing (7)
tests/cli_e2e/mail/helpers_test.gotests/cli_e2e/mail/mail_bot_constraints_test.gotests/cli_e2e/mail/mail_read_reference_test.gotests/cli_e2e/mail/mail_triage_permission_test.gotests/cli_e2e/mail/mail_user_reference_test.gotests/cli_e2e/wiki/helpers_test.gotests/cli_e2e/wiki/wiki_workflow_test.go
Comment on lines
+17
to
+20
| func TestWiki_Workflow_Bot(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | ||
| t.Cleanup(cancel) | ||
|
|
There was a problem hiding this comment.
Add config isolation at test start (Line 17).
This test should isolate CLI config state to avoid cross-test leakage/flakes.
🔧 Suggested patch
func TestWiki_Workflow_Bot(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/wiki/wiki_workflow_test.go` around lines 17 - 20,
TestWiki_Workflow_Bot currently uses shared CLI config and can leak state
between tests; at the start of the TestWiki_Workflow_Bot function add config
isolation by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) so the
test uses a unique temporary config directory, ensuring cleanup via t.TempDir();
place this call immediately after ctx/cancel setup in TestWiki_Workflow_Bot to
isolate CLI config state for the test.
added 2 commits
April 7, 2026 20:42
Change-Id: Ic1f8f4fc4a28499c20154b3f7e3a4b5f4ddc46f2
Change-Id: I966c752cec04d52c6af76348b9227ec9e97f4ca4
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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_core_workflow_test.go`:
- Around line 17-21: The test TestBase_CoreWorkflow currently shells out to the
real CLI and can read/write shared config; isolate its CLI config by setting the
env var LARKSUITE_CLI_CONFIG_DIR to a temp dir for the test (use
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())) before invoking
createBase/uniqueName so the CLI uses the test-specific config directory and
avoids cross-test contamination.
In `@tests/cli_e2e/base/base_dashboard_form_workflow_test.go`:
- Around line 22-25: The dashboard block payloads are referencing the literal
"DashboardTable" instead of the actual generated name created by uniqueName;
update the calls that build block JSON (used by createBlock) to interpolate or
pass the generated table name returned/used by createTable (e.g., store the
uniqueName result in a variable like tableName or use the value returned by
createTable) and replace all occurrences of the string "DashboardTable" in the
block config (including the series.field_name/table_name entries) with that
variable so the block targets the real table; adjust the calls near createTable
and createBlock (and the other occurrences at lines referenced in the comment)
accordingly.
- Around line 17-20: The tests call the real CLI and must isolate shared CLI
config; update the entrypoints (e.g., TestBase_DashboardWorkflow and the other
test referenced around lines 142-145) to set a sandboxed config dir by calling
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start (before any CLI
shell-out) so each test uses its own temporary config directory and cannot
inherit or mutate global auth/state.
In `@tests/cli_e2e/base/base_role_workflow_workflow_test.go`:
- Around line 17-23: In TestBase_RoleAdvpermAndWorkflowCoverage, isolate the CLI
config by setting the env var before any CLI interactions: call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) near the top of the test
(before createBase and any CLI usage) so the test uses an isolated config
directory; update TestBase_RoleAdvpermAndWorkflowCoverage to set this
environment variable (and ensure cleanup via t.TempDir) to avoid sharing host
CLI state.
In `@tests/cli_e2e/base/base_table_record_view_workflow_test.go`:
- Around line 17-25: The test TestBase_TableFieldRecordViewWorkflow runs the
real CLI without isolating its config, causing cross-run state leakage; update
the test to set the environment variable LARKSUITE_CLI_CONFIG_DIR to an isolated
temp directory at the start (use t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir())) so the CLI uses a per-test config directory before calling
createBase/createTable; place this call near the top of
TestBase_TableFieldRecordViewWorkflow so all subsequent CLI invocations use the
isolated config.
- Around line 178-189: The test's assertion using gjson.Get(...,
"data.items.#").Int() >= 0 is weak because a missing path returns 0; update the
"record history list" test to explicitly assert that the "data.items" array
exists (and if expected, that its count is > 0) by checking the presence/type of
the path from result.Stdout before using the count—i.e., replace the current
gjson count check with an assertion that gjson.Get(result.Stdout,
"data.items").Exists() or IsArray(), and then assert gjson.Get(...,
"data.items.#").Int() > 0 if non-empty history is required; keep assertions
using t/require consistent with the surrounding test helpers (result, recordID,
tableID).
In `@tests/cli_e2e/base/helpers_test.go`:
- Around line 51-71: The helpers createBase (and likewise copyBase) currently
provision remote bases but never register cleanup, so add a t.Cleanup
registration inside createBase that calls clie2e.RunCmd to delete the created
base using the returned baseToken (e.g., RunCmd(ctx, clie2e.Request{Args:
[]string{"base", "+base-delete", "--token", baseToken}, DefaultAs: "bot"})) and
assert it succeeds; mirror the same pattern in copyBase to ensure any
created/copied base is removed after the test finishes.
- Around line 381-386: The helper writeTempAttachment currently writes directly
with os.WriteFile; change it to use the repository VFS APIs and validate the
constructed path before writing: call validate.SafeInputPath on the final path
derived from t.TempDir() and the file name, then use vfs.WriteFile (or the
appropriate vfs file write helper) to write the content and check the returned
error; ensure t.Helper(), require.NoError remains and reference
writeTempAttachment, vfs.WriteFile (or vfs.Open/Write), and
validate.SafeInputPath when making the change.
- Around line 133-140: The cleanup handler using parentT.Cleanup calls
clie2e.RunCmd with context.Background() and dereferences
deleteResult.Stdout/Stderr without checking deleteResult; replace
context.Background() with a bounded context (e.g., context.WithTimeout) to avoid
hanging and after calling clie2e.RunCmd check deleteErr and whether deleteResult
is nil before accessing deleteResult.Stdout or deleteResult.Stderr (log a clear
message including deleteErr when deleteResult is nil); apply the same change
pattern to the other Cleanup blocks that call clie2e.RunCmd (lines referencing
deleteResult/deleteErr, parentT.Cleanup, baseToken, tableID).
🪄 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: ff0722d3-b727-4712-bd8e-23b50ca45dd2
📒 Files selected for processing (5)
tests/cli_e2e/base/base_core_workflow_test.gotests/cli_e2e/base/base_dashboard_form_workflow_test.gotests/cli_e2e/base/base_role_workflow_workflow_test.gotests/cli_e2e/base/base_table_record_view_workflow_test.gotests/cli_e2e/base/helpers_test.go
Comment on lines
+17
to
+21
| func TestBase_CoreWorkflow(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) | ||
| t.Cleanup(cancel) | ||
|
|
||
| baseToken := createBase(t, ctx, uniqueName("lark-cli-e2e-base")) |
There was a problem hiding this comment.
Isolate the CLI config for this E2E test.
This shells out to the real CLI without sandboxing LARKSUITE_CLI_CONFIG_DIR, so it can read/write shared local state and cross-contaminate other runs.
Suggested fix
func TestBase_CoreWorkflow(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute)
t.Cleanup(cancel)📝 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 TestBase_CoreWorkflow(t *testing.T) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) | |
| t.Cleanup(cancel) | |
| baseToken := createBase(t, ctx, uniqueName("lark-cli-e2e-base")) | |
| func TestBase_CoreWorkflow(t *testing.T) { | |
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | |
| ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) | |
| t.Cleanup(cancel) | |
| baseToken := createBase(t, ctx, uniqueName("lark-cli-e2e-base")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli_e2e/base/base_core_workflow_test.go` around lines 17 - 21, The test
TestBase_CoreWorkflow currently shells out to the real CLI and can read/write
shared config; isolate its CLI config by setting the env var
LARKSUITE_CLI_CONFIG_DIR to a temp dir for the test (use
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())) before invoking
createBase/uniqueName so the CLI uses the test-specific config directory and
avoids cross-test contamination.
Comment on lines
+17
to
+20
| func TestBase_DashboardWorkflow(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 the CLI config in both E2E entrypoints.
Both tests shell out to the real CLI without sandboxing LARKSUITE_CLI_CONFIG_DIR, so they can inherit and mutate shared config/auth state.
Suggested fix
func TestBase_DashboardWorkflow(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
parentT := t
ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute)
t.Cleanup(cancel)
@@
func TestBase_FormWorkflow(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
parentT := t
ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute)
t.Cleanup(cancel)Also applies to: 142-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli_e2e/base/base_dashboard_form_workflow_test.go` around lines 17 -
20, The tests call the real CLI and must isolate shared CLI config; update the
entrypoints (e.g., TestBase_DashboardWorkflow and the other test referenced
around lines 142-145) to set a sandboxed config dir by calling
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start (before any CLI
shell-out) so each test uses its own temporary config directory and cannot
inherit or mutate global auth/state.
Comment on lines
+17
to
+23
| func TestBase_RoleAdvpermAndWorkflowCoverage(t *testing.T) { | ||
| parentT := t | ||
| ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute) | ||
| t.Cleanup(cancel) | ||
|
|
||
| baseToken := createBase(t, ctx, uniqueName("lark-cli-e2e-base-admin")) | ||
|
|
There was a problem hiding this comment.
Isolate the CLI config for this E2E test.
This workflow uses the real CLI but shares whatever config directory happens to be on the machine, which makes the test stateful across runs.
Suggested fix
func TestBase_RoleAdvpermAndWorkflowCoverage(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
parentT := t
ctx, cancel := context.WithTimeout(context.Background(), 4*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/base/base_role_workflow_workflow_test.go` around lines 17 - 23,
In TestBase_RoleAdvpermAndWorkflowCoverage, isolate the CLI config by setting
the env var before any CLI interactions: call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) near the top of the test
(before createBase and any CLI usage) so the test uses an isolated config
directory; update TestBase_RoleAdvpermAndWorkflowCoverage to set this
environment variable (and ensure cleanup via t.TempDir) to avoid sharing host
CLI state.
Comment on lines
+17
to
+25
| func TestBase_TableFieldRecordViewWorkflow(t *testing.T) { | ||
| parentT := t | ||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) | ||
| t.Cleanup(cancel) | ||
|
|
||
| baseToken := createBase(t, ctx, uniqueName("lark-cli-e2e-base-main")) | ||
| tableID, primaryFieldID, primaryViewID := createTable(t, parentT, ctx, baseToken, uniqueName("Orders"), `[{"name":"Name","type":"text"}]`, `{"name":"Main","type":"grid"}`) | ||
| require.NotEmpty(t, primaryFieldID) | ||
| require.NotEmpty(t, primaryViewID) |
There was a problem hiding this comment.
Isolate the CLI config for this E2E test.
This test invokes the real CLI but never overrides LARKSUITE_CLI_CONFIG_DIR, so it can bleed auth/config state across runs.
Suggested fix
func TestBase_TableFieldRecordViewWorkflow(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
parentT := t
ctx, cancel := context.WithTimeout(context.Background(), 5*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/base/base_table_record_view_workflow_test.go` around lines 17 -
25, The test TestBase_TableFieldRecordViewWorkflow runs the real CLI without
isolating its config, causing cross-run state leakage; update the test to set
the environment variable LARKSUITE_CLI_CONFIG_DIR to an isolated temp directory
at the start (use t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())) so the CLI
uses a per-test config directory before calling createBase/createTable; place
this call near the top of TestBase_TableFieldRecordViewWorkflow so all
subsequent CLI invocations use the isolated config.
Comment on lines
+381
to
+386
| func writeTempAttachment(t *testing.T, content string) string { | ||
| t.Helper() | ||
|
|
||
| path := filepath.Join(t.TempDir(), "attachment.txt") | ||
| err := os.WriteFile(path, []byte(content), 0o644) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the repo filesystem abstraction for temp attachments.
This helper writes through os.WriteFile without path validation. Please route this through vfs.* and validate the path before writing. As per coding guidelines, **/*.go: Use vfs.* instead of os.* for all filesystem access and validate paths using validate.SafeInputPath before any file I/O operations.
🤖 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 381 - 386, The helper
writeTempAttachment currently writes directly with os.WriteFile; change it to
use the repository VFS APIs and validate the constructed path before writing:
call validate.SafeInputPath on the final path derived from t.TempDir() and the
file name, then use vfs.WriteFile (or the appropriate vfs file write helper) to
write the content and check the returned error; ensure t.Helper(),
require.NoError remains and reference writeTempAttachment, vfs.WriteFile (or
vfs.Open/Write), and validate.SafeInputPath when making the change.
Change-Id: Id5df27eca7c99c0d0caa46b29b64be30e6f7a29a
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
tests/cli_e2e/base/helpers_test.go (3)
191-198:⚠️ Potential issue | 🟠 MajorBound cleanup CLI calls with a timeout.
Each cleanup uses
context.Background()for external command execution. If CLI hangs during teardown, the test can hang indefinitely.Suggested pattern
- deleteResult, deleteErr := clie2e.RunCmd(context.Background(), clie2e.Request{ + cleanupCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + deleteResult, deleteErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{Also applies to: 224-231, 257-264, 290-297, 320-327, 350-357, 380-387, 440-447
🤖 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 191 - 198, The cleanup calls use context.Background() which can hang; update each parentT.Cleanup invocation that calls clie2e.RunCmd (e.g., in the block referencing parentT.Cleanup, clie2e.RunCmd, clie2e.Request and reportCleanupFailure) to create a cancellable context with a timeout (context.WithTimeout) and defer cancel(), pass that context into clie2e.RunCmd, and handle the timeout error path the same way you handle deleteErr so the test teardown cannot hang indefinitely; apply the same change to the other listed cleanup blocks.
472-483: 🛠️ Refactor suggestion | 🟠 MajorUse
vfs.*+ safe path validation for temp attachments.This helper performs direct filesystem I/O via
os.*and writes to the working directory withoutvalidate.SafeInputPath.Suggested 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") - err = os.WriteFile(path, []byte(content), 0o644) + path := filepath.Join(t.TempDir(), "attachment-"+testSuffix()+".txt") + safePath, err := validate.SafeInputPath(path) + require.NoError(t, err) + err = vfs.WriteFile(safePath, []byte(content), 0o644) require.NoError(t, err) - t.Cleanup(func() { - _ = os.Remove(path) - }) return "./" + filepath.Base(path) }As per coding guidelines
**/*.go: "Usevfs.*instead ofos.*for all filesystem access" and "Validate paths usingvalidate.SafeInputPathbefore any file I/O operations."🤖 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 472 - 483, The helper writeTempAttachment currently uses os.Getwd, filepath.Join and os.WriteFile/Remove directly; change it to use the project's VFS API (vfs.*) for all filesystem operations and validate the generated path with validate.SafeInputPath before writing or deleting. Specifically, generate the filename using testSuffix(), build the path, call validate.SafeInputPath(path) and then use vfs.WriteFile (or vfs.OpenFile with proper flags) to create the file and vfs.Remove in t.Cleanup to delete it; keep the function name writeTempAttachment and preserve t.Helper()/t.Cleanup behavior and require.NoError checks around vfs operations.
109-129:⚠️ Potential issue | 🟠 MajorAdd teardown for created/copied bases.
createBaseandcopyBaseprovision remote resources but never register cleanup, so test runs leak bases and become state/quota dependent over time.Suggested fix
func createBase(t *testing.T, ctx context.Context, name string) string { @@ require.NotEmpty(t, baseToken, "stdout:\n%s", result.Stdout) + t.Cleanup(func() { + cleanupCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + deleteResult, deleteErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{ + Args: []string{"base", "+base-delete", "--token", baseToken, "--yes"}, + DefaultAs: "bot", + }) + if deleteErr != nil || deleteResult.ExitCode != 0 { + reportCleanupFailure(t, "delete base "+baseToken, deleteResult, deleteErr) + } + }) return baseToken }Also applies to: 131-151
🤖 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 109 - 129, createBase and copyBase currently provision remote bases but never clean them up; modify both functions (createBase and copyBase) to register a teardown with t.Cleanup that deletes the created base when the test finishes. After extracting the base token/ID from result.Stdout (the existing baseToken variable), call clie2e.RunCmd (or the existing client helper) with the appropriate "base delete" CLI request (using the app/base token or id you obtained) inside the cleanup closure and assert/ignore non-fatal errors; ensure the cleanup closure captures the baseToken variable so each test run removes its created/copied base.
🤖 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_advperm_workflow_test.go`:
- Around line 20-25: In TestBase_AdvpermWorkflow, isolate CLI config state by
calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of the
test (before createBase and other actions) so the test uses a unique per-test
config directory; add this single line near the top of the
TestBase_AdvpermWorkflow function (e.g., immediately after context setup) to
prevent cross-test/config pollution.
In `@tests/cli_e2e/base/base_role_workflow_test.go`:
- Around line 22-26: In TestBase_RoleWorkflow, set isolated config state before
creating the execution context by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) at the top of the test (before ctx, cancel :=
context.WithTimeout(...)), so the test uses a per-run config directory; update
the TestBase_RoleWorkflow function accordingly to ensure config isolation.
In `@tests/cli_e2e/base/base_workflow_lifecycle_test.go`:
- Around line 22-26: The TestBase_WorkflowLifecycle test does not isolate CLI
config which can lead to non-deterministic E2E runs; at the start of
TestBase_WorkflowLifecycle (function TestBase_WorkflowLifecycle) call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to point
LARKSUITE_CLI_CONFIG_DIR to a fresh temporary directory for this test so config
state is isolated and deterministic.
In `@tests/cli_e2e/wiki/helpers_test.go`:
- Around line 68-69: The test is parsing JSON directly from result.Stdout with
gjson.Get which can fail if the CLI emits non-JSON output before the envelope;
replace the direct gjson.Get(result.Stdout, ...) usage by first extracting the
clean JSON with wikiJSONPayload(result.Stdout) and then call gjson.Get on that
payload (i.e., use gjson.Get(wikiJSONPayload(result.Stdout), "data.node")),
keeping the existing require.True assertion and message unchanged.
---
Duplicate comments:
In `@tests/cli_e2e/base/helpers_test.go`:
- Around line 191-198: The cleanup calls use context.Background() which can
hang; update each parentT.Cleanup invocation that calls clie2e.RunCmd (e.g., in
the block referencing parentT.Cleanup, clie2e.RunCmd, clie2e.Request and
reportCleanupFailure) to create a cancellable context with a timeout
(context.WithTimeout) and defer cancel(), pass that context into clie2e.RunCmd,
and handle the timeout error path the same way you handle deleteErr so the test
teardown cannot hang indefinitely; apply the same change to the other listed
cleanup blocks.
- Around line 472-483: The helper writeTempAttachment currently uses os.Getwd,
filepath.Join and os.WriteFile/Remove directly; change it to use the project's
VFS API (vfs.*) for all filesystem operations and validate the generated path
with validate.SafeInputPath before writing or deleting. Specifically, generate
the filename using testSuffix(), build the path, call
validate.SafeInputPath(path) and then use vfs.WriteFile (or vfs.OpenFile with
proper flags) to create the file and vfs.Remove in t.Cleanup to delete it; keep
the function name writeTempAttachment and preserve t.Helper()/t.Cleanup behavior
and require.NoError checks around vfs operations.
- Around line 109-129: createBase and copyBase currently provision remote bases
but never clean them up; modify both functions (createBase and copyBase) to
register a teardown with t.Cleanup that deletes the created base when the test
finishes. After extracting the base token/ID from result.Stdout (the existing
baseToken variable), call clie2e.RunCmd (or the existing client helper) with the
appropriate "base delete" CLI request (using the app/base token or id you
obtained) inside the cleanup closure and assert/ignore non-fatal errors; ensure
the cleanup closure captures the baseToken variable so each test run removes its
created/copied base.
🪄 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: e25081ff-adf9-4a12-8951-9ca24729ca96
📒 Files selected for processing (15)
tests/cli_e2e/COVERAGE.mdtests/cli_e2e/base/base_advperm_workflow_test.gotests/cli_e2e/base/base_core_workflow_test.gotests/cli_e2e/base/base_dashboard_form_workflow_test.gotests/cli_e2e/base/base_role_workflow_test.gotests/cli_e2e/base/base_table_record_view_workflow_test.gotests/cli_e2e/base/base_workflow_lifecycle_test.gotests/cli_e2e/base/helpers_test.gotests/cli_e2e/task/task_comment_workflow_test.gotests/cli_e2e/task/task_reminder_workflow_test.gotests/cli_e2e/task/task_status_workflow_test.gotests/cli_e2e/task/tasklist_add_task_workflow_test.gotests/cli_e2e/task/tasklist_workflow_test.gotests/cli_e2e/wiki/helpers_test.gotests/cli_e2e/wiki/wiki_workflow_test.go
✅ Files skipped from review due to trivial changes (7)
- tests/cli_e2e/task/task_comment_workflow_test.go
- tests/cli_e2e/task/tasklist_add_task_workflow_test.go
- tests/cli_e2e/task/tasklist_workflow_test.go
- tests/cli_e2e/task/task_reminder_workflow_test.go
- tests/cli_e2e/task/task_status_workflow_test.go
- tests/cli_e2e/COVERAGE.md
- tests/cli_e2e/base/base_core_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/cli_e2e/base/base_table_record_view_workflow_test.go
- tests/cli_e2e/wiki/wiki_workflow_test.go
- tests/cli_e2e/base/base_dashboard_form_workflow_test.go
Comment on lines
+20
to
+25
| func TestBase_AdvpermWorkflow(t *testing.T) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute) | ||
| t.Cleanup(cancel) | ||
|
|
||
| baseToken := createBase(t, ctx, "lark-cli-e2e-base-advperm-"+testSuffix()) | ||
|
|
There was a problem hiding this comment.
Isolate CLI config state for this E2E test.
Please set a per-test config directory at test start to avoid cross-test/config pollution.
func TestBase_AdvpermWorkflow(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
ctx, cancel := context.WithTimeout(context.Background(), 4*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 TestBase_AdvpermWorkflow(t *testing.T) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute) | |
| t.Cleanup(cancel) | |
| baseToken := createBase(t, ctx, "lark-cli-e2e-base-advperm-"+testSuffix()) | |
| func TestBase_AdvpermWorkflow(t *testing.T) { | |
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | |
| ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute) | |
| t.Cleanup(cancel) | |
| baseToken := createBase(t, ctx, "lark-cli-e2e-base-advperm-"+testSuffix()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli_e2e/base/base_advperm_workflow_test.go` around lines 20 - 25, In
TestBase_AdvpermWorkflow, isolate CLI config state by calling
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of the test
(before createBase and other actions) so the test uses a unique per-test config
directory; add this single line near the top of the TestBase_AdvpermWorkflow
function (e.g., immediately after context setup) to prevent cross-test/config
pollution.
Comment on lines
+22
to
+26
| 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.
Set isolated config dir in test setup.
Add per-test config isolation before creating the execution context.
func TestBase_RoleWorkflow(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
parentT := t
ctx, cancel := context.WithTimeout(context.Background(), 4*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 TestBase_RoleWorkflow(t *testing.T) { | |
| parentT := t | |
| ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute) | |
| t.Cleanup(cancel) | |
| func TestBase_RoleWorkflow(t *testing.T) { | |
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | |
| parentT := t | |
| ctx, cancel := context.WithTimeout(context.Background(), 4*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/base/base_role_workflow_test.go` around lines 22 - 26, In
TestBase_RoleWorkflow, set isolated config state before creating the execution
context by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the top
of the test (before ctx, cancel := context.WithTimeout(...)), so the test uses a
per-run config directory; update the TestBase_RoleWorkflow function accordingly
to ensure config isolation.
Comment on lines
+22
to
+26
| func TestBase_WorkflowLifecycle(t *testing.T) { | ||
| parentT := t | ||
| ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute) | ||
| t.Cleanup(cancel) | ||
|
|
There was a problem hiding this comment.
Add config isolation for deterministic E2E behavior.
Set LARKSUITE_CLI_CONFIG_DIR to a temp dir at the start of this test.
func TestBase_WorkflowLifecycle(t *testing.T) {
+ t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
parentT := t
ctx, cancel := context.WithTimeout(context.Background(), 4*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 TestBase_WorkflowLifecycle(t *testing.T) { | |
| parentT := t | |
| ctx, cancel := context.WithTimeout(context.Background(), 4*time.Minute) | |
| t.Cleanup(cancel) | |
| func TestBase_WorkflowLifecycle(t *testing.T) { | |
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | |
| parentT := t | |
| ctx, cancel := context.WithTimeout(context.Background(), 4*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/base/base_workflow_lifecycle_test.go` around lines 22 - 26, The
TestBase_WorkflowLifecycle test does not isolate CLI config which can lead to
non-deterministic E2E runs; at the start of TestBase_WorkflowLifecycle (function
TestBase_WorkflowLifecycle) call t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) to point LARKSUITE_CLI_CONFIG_DIR to a fresh temporary directory
for this test so config state is isolated and deterministic.
Change-Id: I49dc33a0f6639a7ca380373973f0cc2d874e34cd
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/cli_e2e/base/base_workflow_lifecycle_test.go (1)
27-31:⚠️ Potential issue | 🟠 MajorAdd test config isolation at test start.
At Line 27, this test still does not isolate CLI config state. Add
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())at the beginning ofTestBase_WorkflowLifecycleto avoid machine-local config bleed-through in E2E runs.Suggested patch
func TestBase_WorkflowLifecycle(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) parentT := t ctx, cancel := context.WithTimeout(context.Background(), 4*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/base/base_workflow_lifecycle_test.go` around lines 27 - 31, The test TestBase_WorkflowLifecycle does not isolate CLI config state and may leak machine-local config; at the start of the TestBase_WorkflowLifecycle function add t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) so the test uses a fresh temporary config directory for the run and avoids config bleed-through in E2E tests.tests/cli_e2e/base/base_table_record_view_workflow_test.go (2)
49-54:⚠️ Potential issue | 🟠 MajorStill missing per-test CLI config isolation.
This workflow starts provisioning through the real CLI immediately, but it never overrides
LARKSUITE_CLI_CONFIG_DIR. That makes the result depend on machine-local auth/cache state and can leak config across runs. Set the env var beforecreateBase(...).Suggested fix
func TestBase_TableFieldRecordViewWorkflow(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) parentT := t ctx, cancel := context.WithTimeout(context.Background(), 5*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/base/base_table_record_view_workflow_test.go` around lines 49 - 54, The test TestBase_TableFieldRecordViewWorkflow currently invokes createBase(t, ctx, ...) without isolating CLI config; before calling createBase, set the environment for the test using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) so the CLI uses a test-local config directory; update the TestBase_TableFieldRecordViewWorkflow setup (before the createBase call) to call t.Setenv with t.TempDir() to ensure per-test CLI config isolation.
213-224:⚠️ Potential issue | 🟡 Minor
record history liststill has a vacuous assertion.
gjson.Get(..., "data.items.#").Int() >= 0also passes whendata.itemsis missing, because the missing count reads as0. This subtest should at least assert thatdata.itemsexists before checking its size.Minimal fix
- assert.True(t, gjson.Get(result.Stdout, "data.items.#").Int() >= 0, "stdout:\n%s", result.Stdout) + assert.True(t, gjson.Get(result.Stdout, "data.items").Exists(), "stdout:\n%s", result.Stdout)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/base/base_table_record_view_workflow_test.go` around lines 213 - 224, The subtest "record history list" uses a vacuous size check; first assert that the "data.items" field actually exists before checking its count. In the test body around the call that produces result (the clie2e.RunCmd call and the variables result/recordID/tableID), add an assertion like require.True(t, gjson.Get(result.Stdout, "data.items").Exists()) (or require.NotEmpty on the raw JSON) and then keep the count assertion (gjson.Get(..., "data.items.#").Int() >= 0) so the size check only runs when the field is present.
🧹 Nitpick comments (1)
tests/cli_e2e/base/base_table_record_view_workflow_test.go (1)
242-253: These read-back subtests only prove the commands didn’t error.
data queryand theview get group/sort/timebar/cardblocks never inspect the returned payload, so a no-op setter or wrong serializer can still pass. Since this test already knows the expected values (Closed,statusFieldID,dueFieldID,dueEndFieldID,primaryFieldID,attachmentFieldID), add at least one semantic round-trip assertion per block.Also applies to: 322-344, 346-368, 370-392, 394-416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/base/base_table_record_view_workflow_test.go` around lines 242 - 253, The tests only check exit codes and not the returned payload, so update the t.Run blocks (e.g., the "data query" block and the "view get group/sort/timebar/card" blocks) to parse the command output and assert at least one semantic round-trip value using known expectations: assert the returned dimension/measure/field values include "Closed" and that field IDs/keys match statusFieldID, dueFieldID, dueEndFieldID, primaryFieldID and attachmentFieldID; specifically, after calling clie2e.RunCmd in each block, decode the JSON result (from result.Stdout or equivalent) and add assertions that the relevant JSON fields (dimensions[], measures[], view.group/sort/timebar/card fields) contain the expected names/IDs to ensure the serializer/setter actually returned correct data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cli_e2e/task/task_get_my_tasks_user_test.go`:
- Around line 23-26: The TestTask_GetMyTasks_User test does not isolate CLI
config and should set a per-test config dir; update the TestTask_GetMyTasks_User
function to call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) early in the
test (after creating ctx/cancel) so the test uses a temporary config directory
and avoids leaking/shared auth state across tests.
---
Duplicate comments:
In `@tests/cli_e2e/base/base_table_record_view_workflow_test.go`:
- Around line 49-54: The test TestBase_TableFieldRecordViewWorkflow currently
invokes createBase(t, ctx, ...) without isolating CLI config; before calling
createBase, set the environment for the test using
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) so the CLI uses a test-local
config directory; update the TestBase_TableFieldRecordViewWorkflow setup (before
the createBase call) to call t.Setenv with t.TempDir() to ensure per-test CLI
config isolation.
- Around line 213-224: The subtest "record history list" uses a vacuous size
check; first assert that the "data.items" field actually exists before checking
its count. In the test body around the call that produces result (the
clie2e.RunCmd call and the variables result/recordID/tableID), add an assertion
like require.True(t, gjson.Get(result.Stdout, "data.items").Exists()) (or
require.NotEmpty on the raw JSON) and then keep the count assertion
(gjson.Get(..., "data.items.#").Int() >= 0) so the size check only runs when the
field is present.
In `@tests/cli_e2e/base/base_workflow_lifecycle_test.go`:
- Around line 27-31: The test TestBase_WorkflowLifecycle does not isolate CLI
config state and may leak machine-local config; at the start of the
TestBase_WorkflowLifecycle function add t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) so the test uses a fresh temporary config directory for the run and
avoids config bleed-through in E2E tests.
---
Nitpick comments:
In `@tests/cli_e2e/base/base_table_record_view_workflow_test.go`:
- Around line 242-253: The tests only check exit codes and not the returned
payload, so update the t.Run blocks (e.g., the "data query" block and the "view
get group/sort/timebar/card" blocks) to parse the command output and assert at
least one semantic round-trip value using known expectations: assert the
returned dimension/measure/field values include "Closed" and that field IDs/keys
match statusFieldID, dueFieldID, dueEndFieldID, primaryFieldID and
attachmentFieldID; specifically, after calling clie2e.RunCmd in each block,
decode the JSON result (from result.Stdout or equivalent) and add assertions
that the relevant JSON fields (dimensions[], measures[],
view.group/sort/timebar/card fields) contain the expected names/IDs to ensure
the serializer/setter actually returned correct data.
🪄 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: 1a37b578-cc28-45ff-b20a-0b1d5ca526a2
📒 Files selected for processing (13)
tests/cli_e2e/base/base_advperm_workflow_test.gotests/cli_e2e/base/base_core_workflow_test.gotests/cli_e2e/base/base_dashboard_form_workflow_test.gotests/cli_e2e/base/base_role_workflow_test.gotests/cli_e2e/base/base_table_record_view_workflow_test.gotests/cli_e2e/base/base_workflow_lifecycle_test.gotests/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/wiki_workflow_test.go
✅ Files skipped from review due to trivial changes (5)
- tests/cli_e2e/task/tasklist_add_task_workflow_test.go
- tests/cli_e2e/task/task_reminder_workflow_test.go
- tests/cli_e2e/task/task_comment_workflow_test.go
- tests/cli_e2e/task/tasklist_workflow_test.go
- tests/cli_e2e/task/task_status_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/cli_e2e/base/base_core_workflow_test.go
- tests/cli_e2e/base/base_advperm_workflow_test.go
- tests/cli_e2e/base/base_role_workflow_test.go
- tests/cli_e2e/wiki/wiki_workflow_test.go
- tests/cli_e2e/base/base_dashboard_form_workflow_test.go
Comment on lines
+23
to
+26
| func TestTask_GetMyTasks_User(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 per test to prevent cross-test state leakage.
At Line 23, the test starts without setting LARKSUITE_CLI_CONFIG_DIR to a temp dir. This can make E2E results depend on external/shared auth config.
Suggested patch
func TestTask_GetMyTasks_User(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 TestTask_GetMyTasks_User(t *testing.T) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) | |
| t.Cleanup(cancel) | |
| func TestTask_GetMyTasks_User(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/task/task_get_my_tasks_user_test.go` around lines 23 - 26, The
TestTask_GetMyTasks_User test does not isolate CLI config and should set a
per-test config dir; update the TestTask_GetMyTasks_User function to call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) early in the test (after
creating ctx/cancel) so the test uses a temporary config directory and avoids
leaking/shared auth state across tests.
…/cli into feat/cli-shortcut-e2e-tests Change-Id: Id9b41f5297067ad7fbc00b4d668a7458d5a08058
Change-Id: I6ddaa82e399a8baba277232bdd71a33eea262958
github-actions
Bot
added
size/M
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/coverage.md`:
- Around line 18-20: Add a single blank line immediately before the "## Command
Table" header so the table has a blank line separating it from the previous
content (this satisfies markdownlint MD058); locate the "## Command Table"
header in the tests/cli_e2e/base/coverage.md content and insert one empty line
above it.
In `@tests/cli_e2e/cli-e2e-testcase-writer/SKILL.md`:
- Line 107: Update the phrasing "Write testcase entries in `go test -run`
friendly form." to use hyphenated wording for clarity and compatibility by
changing it to "Write testcase entries in `go test -run`-friendly form." — edit
the SKILL.md line containing that exact sentence to insert the hyphen between
`-run` and friendly.
In `@tests/cli_e2e/task/coverage.md`:
- Around line 19-21: Add a single blank line between the "## Command Table"
heading and the subsequent table header row to satisfy markdownlint MD058; edit
the file section containing the heading "## Command Table" and the table start
line "| Status | Cmd | Type | Testcase | Key parameter shapes | Notes /
uncovered reason |" so there is one empty line separating them.
- Around line 42-49: Update the awkward phrasing "needs isolated ..." in the
coverage table entries for "task tasklists list" and "task tasks list" to use
standard wording; locate the two rows containing "needs isolated list or filter
assertions against ambient tasklist data" (the row starting with "task tasklists
list") and the row for "task tasks list" and replace that fragment with "needs
dedicated list or filter assertions against ambient task data" (or use "needs to
be isolated with list or filter assertions" if preferred) so the entries read
clearly and consistently.
🪄 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: 73569a13-cb4a-43bc-9426-43ef0b4c8ad0
📒 Files selected for processing (6)
.gitignoretests/cli_e2e/base/coverage.mdtests/cli_e2e/cli-e2e-testcase-writer/SKILL.mdtests/cli_e2e/demo/coverage.mdtests/cli_e2e/task/coverage.mdtests/cli_e2e/wiki/coverage.md
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- tests/cli_e2e/wiki/coverage.md
…into feat/e2e-tests
yxzhaao
changed the title
Change-Id: Id66f292991898650a6fa83bd225e83f687e4ee22
…ummary Change-Id: Ie04e887811f34a1f99dd9fb9d08b68db9b3f176a
Change-Id: Iae2774707ea6c7ba11717f0f711f72f8898e09ea
jinzemo
force-pushed
the
feat/cli-shortcut-e2e-tests
branch
from
5d0c010 to
8477a8c
Compare
Change-Id: If8dc8da97977f1be869ebfe48cd69350d77694ed
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.
Change-Id: I43922a6cce5a671e842e48e57e8fb77aae738647
Summary
Changes
Test Plan
lark xxxcommand works as expectedRelated Issues
Summary by CodeRabbit
Release Notes
Tests
Documentation
Chores
.gitignoreto exclude application logs.