From 199cd2f6ac707eca4beb6eb2389f5b1404782ad3 Mon Sep 17 00:00:00 2001 From: fangshuyu-768 Date: Tue, 21 Apr 2026 11:58:25 +0800 Subject: [PATCH] fix(doc): address CodeRabbit review on docs +update warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CodeRabbit nits from #569: 1. Unit test hint assertion only checked for `delete_range` in the remediation message; the companion `insert_before` half of the guidance could regress undetected. Broaden the assertion to require both tokens so a future edit that drops half the remediation produces an immediate test failure. 2. No E2E coverage proved the dry-run contract in the PR description ("Not emitted in dry-run mode — kept quiet during planning"). The helper itself is unit-tested, but nothing caught a regression where a later refactor wired docsUpdateWarnings into the DryRun path. Add tests/cli_e2e/docs/docs_update_dryrun_test.go: TestDocs_UpdateDryRunSuppressesSemanticWarnings invokes `docs +update --dry-run --mode=replace_range --markdown "***x***\n\nb"` — an input crafted to trip BOTH pre-write warnings — and asserts neither the "warning:" prefix, the blank-line message, nor the combined-emphasis message appears on stdout or stderr. Note: the file needs -f to add because .gitignore has a bare `docs/` rule that accidentally matches tests/cli_e2e/docs/. The existing tracked files under that directory predate the rule; new additions have to be force-added until the ignore pattern is narrowed. Not worth rewriting .gitignore for one file. Verified manually that the new E2E fails cleanly when warnings are injected into DryRun and passes again after reverting — the test has real regression-detection power, not just a sticker. --- shortcuts/doc/docs_update_check_test.go | 2 +- tests/cli_e2e/docs/docs_update_dryrun_test.go | 70 +++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 tests/cli_e2e/docs/docs_update_dryrun_test.go diff --git a/shortcuts/doc/docs_update_check_test.go b/shortcuts/doc/docs_update_check_test.go index 6208e0540..50905873a 100644 --- a/shortcuts/doc/docs_update_check_test.go +++ b/shortcuts/doc/docs_update_check_test.go @@ -147,7 +147,7 @@ func TestCheckDocsUpdateReplaceMultilineMarkdown(t *testing.T) { t.Fatalf("checkDocsUpdateReplaceMultilineMarkdown(%q, %q) = %q, wantHint=%v", tt.mode, tt.markdown, got, tt.wantHint) } - if tt.wantHint && !strings.Contains(got, "delete_range") { + if tt.wantHint && (!strings.Contains(got, "delete_range") || !strings.Contains(got, "insert_before")) { t.Errorf("hint should suggest delete_range/insert_before remediation, got: %s", got) } }) diff --git a/tests/cli_e2e/docs/docs_update_dryrun_test.go b/tests/cli_e2e/docs/docs_update_dryrun_test.go new file mode 100644 index 000000000..6e0f8cce3 --- /dev/null +++ b/tests/cli_e2e/docs/docs_update_dryrun_test.go @@ -0,0 +1,70 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package docs + +import ( + "context" + "strings" + "testing" + "time" + + clie2e "github.com/larksuite/cli/tests/cli_e2e" + "github.com/stretchr/testify/require" +) + +// TestDocs_UpdateDryRunSuppressesSemanticWarnings asserts the contract that +// docsUpdateWarnings is NOT invoked on the --dry-run path. The unit tests in +// shortcuts/doc/docs_update_check_test.go prove the helper emits warnings for +// replace_range + blank-line and for combined-emphasis markers; this E2E +// locks in that they never reach the user during dry-run planning, so a +// future refactor that moves warning emission into a shared code path can't +// silently regress. +// +// Input is intentionally crafted to trigger BOTH warnings the helper emits: +// - mode=replace_range + markdown containing "\n\n" (blank-line warning) +// - markdown containing `***combined***` (combined bold+italic warning) +// +// Neither string may appear in dry-run output. +func TestDocs_UpdateDryRunSuppressesSemanticWarnings(t *testing.T) { + // Fake creds are enough — dry-run short-circuits before any real API call. + t.Setenv("LARKSUITE_CLI_APP_ID", "app") + t.Setenv("LARKSUITE_CLI_APP_SECRET", "secret") + t.Setenv("LARKSUITE_CLI_BRAND", "feishu") + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) + + // "***combined***" is a triple-asterisk combined-emphasis shape; "\n\n" + // is a paragraph break. Both would normally produce warnings when + // Execute runs under --mode=replace_range; both must be absent here. + markdown := "***combined***\n\nsecond paragraph" + + result, err := clie2e.RunCmd(ctx, clie2e.Request{ + Args: []string{ + "docs", "+update", + "--doc", "doxcnDryRunE2E", + "--mode", "replace_range", + "--selection-with-ellipsis", "placeholder", + "--markdown", markdown, + "--dry-run", + }, + DefaultAs: "bot", + }) + require.NoError(t, err) + result.AssertExitCode(t, 0) + + // Neither warning prefix ("warning:") nor either specific warning body + // may appear in dry-run output (stdout OR stderr). + combined := result.Stdout + "\n" + result.Stderr + for _, needle := range []string{ + "warning:", + "does not split a block into multiple paragraphs", + "combined bold+italic markers", + } { + if strings.Contains(combined, needle) { + t.Errorf("dry-run output must not surface pre-write warning %q\nstdout:\n%s\nstderr:\n%s", + needle, result.Stdout, result.Stderr) + } + } +}