Skip to content

fix e2e pagination assumptions#639

Merged
liangshuo-1 merged 1 commit intolarksuite:mainfrom
yxzhaao:fix/fix-e2e-pagination
Apr 23, 2026
Merged

fix e2e pagination assumptions#639
liangshuo-1 merged 1 commit intolarksuite:mainfrom
yxzhaao:fix/fix-e2e-pagination

Conversation

@yxzhaao
Copy link
Copy Markdown
Contributor

@yxzhaao yxzhaao commented Apr 23, 2026

Summary

Changes

  • Change 1
  • Change 2

Test Plan

  • Unit tests pass
  • Manual local verification confirms the lark xxx command works as expected

Related Issues

  • None

Summary by CodeRabbit

  • Tests

    • Refactored end-to-end tests to use new helper functions for locating resources (tables, calendars, tasks) with improved pagination handling and error reporting.
  • Refactor

    • Consolidated common resource lookup logic into dedicated test helpers, reducing code duplication and improving test maintainability.

@github-actions github-actions Bot added the size/S Low-risk docs, CI, test, or chore only changes label Apr 23, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

📝 Walkthrough

Walkthrough

E2E tests across base, calendar, and task modules are refactored to use new pagination-aware helper functions for resource lookup instead of executing CLI commands inline. Three new helpers (findBaseTableByID, findCalendarByID, findTaskInTasklist) encapsulate repeated pagination and parsing logic.

Changes

Cohort / File(s) Summary
Base table resource lookup
tests/cli_e2e/base/helpers_test.go, tests/cli_e2e/base/base_basic_workflow_test.go
New helper findBaseTableByID implements paginated table lookup with loop detection. Test refactored to use helper instead of inline CLI command execution.
Calendar resource lookup
tests/cli_e2e/calendar/helpers_test.go, tests/cli_e2e/calendar/calendar_manage_calendar_test.go
New helper findCalendarByID implements paginated calendar lookup with page token tracking. Test refactored to use helper for calendar validation.
Task resource lookup
tests/cli_e2e/task/helpers_test.go, tests/cli_e2e/task/tasklist_add_task_workflow_test.go, tests/cli_e2e/task/tasklist_workflow_test.go
New helper findTaskInTasklist implements paginated task lookup with loop detection. Two tests refactored to delegate task lookup to helper.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

Helpers hop through pages with care,
Pagination loops? They'll beware!
Tests now lean on steady aids,
As CLI parsing fades,
Refactored code stands bright and fair. 🐰

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description uses the template structure but contains only placeholder content: empty Summary, generic 'Change 1/2' items, unchecked Test Plan items, and no actual motivation or implementation details. Fill in the Summary with motivation for pagination assumptions fix, replace Change 1/2 with specific helper functions added, and provide actual test verification details.
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix e2e pagination assumptions' directly reflects the main change: refactoring e2e tests to remove direct pagination command execution in favor of helper functions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch fix/fix-e2e-pagination

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/cli_e2e/task/helpers_test.go (1)

71-111: Helper looks solid; pagination & loop detection mirror findCalendarByID.

Non-empty guards, seenPageTokens loop detection, and the hasMore || pageToken == "" termination are all consistent with the calendar helper. Using AssertStdoutStatus(t, 0) for a list response matches the pattern used elsewhere in tasklist_workflow_test.go (line 87).

One optional nit: findBaseTableByID, findCalendarByID, and findTaskInTasklist now share ~90% of the pagination/loop-detection structure across three packages. If this pattern grows further, consider extracting a small generic helper (e.g., in tests/cli_e2e) that takes the command args, item JSON path, and match predicate, so future pagination fixes only need to land in one place. Safe to defer.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/cli_e2e/task/helpers_test.go` around lines 71 - 111, Extract the
duplicated pagination + loop-detection logic used in findBaseTableByID,
findCalendarByID, and findTaskInTasklist into a shared helper in tests/cli_e2e
(e.g., PaginatedFind or RunPaginatedLookup) that accepts the context, command
args, base params (page_size etc.), the gjson path to select items, and a
predicate (or target GUID) to detect a match; then rewrite findBaseTableByID,
findCalendarByID, and findTaskInTasklist to call this helper passing their
specific Args and JSON path, preserving the seenPageTokens loop-detection,
has_more/page_token handling, and existing assertions
(AssertExitCode/AssertStdoutStatus) so behavior remains identical.
🤖 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/helpers_test.go`:
- Around line 173-207: The test uses a hardcoded "--limit", causing mismatch
with the pageLimit constant; update the args construction in the loop where
clie2e.RunCmd is called (the args slice containing "--limit", "50") to use
strconv.Itoa(pageLimit) instead of the literal "50" so the pagination check
using pageLimit and the len(tables) < pageLimit heuristic remain consistent with
offset and seenOffsets logic.

---

Nitpick comments:
In `@tests/cli_e2e/task/helpers_test.go`:
- Around line 71-111: Extract the duplicated pagination + loop-detection logic
used in findBaseTableByID, findCalendarByID, and findTaskInTasklist into a
shared helper in tests/cli_e2e (e.g., PaginatedFind or RunPaginatedLookup) that
accepts the context, command args, base params (page_size etc.), the gjson path
to select items, and a predicate (or target GUID) to detect a match; then
rewrite findBaseTableByID, findCalendarByID, and findTaskInTasklist to call this
helper passing their specific Args and JSON path, preserving the seenPageTokens
loop-detection, has_more/page_token handling, and existing assertions
(AssertExitCode/AssertStdoutStatus) so behavior remains identical.
🪄 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: 60ee8331-b5b9-49cc-bce9-047a4351836e

📥 Commits

Reviewing files that changed from the base of the PR and between 600fa50 and ccbdde5.

📒 Files selected for processing (7)
  • tests/cli_e2e/base/base_basic_workflow_test.go
  • tests/cli_e2e/base/helpers_test.go
  • tests/cli_e2e/calendar/calendar_manage_calendar_test.go
  • tests/cli_e2e/calendar/helpers_test.go
  • tests/cli_e2e/task/helpers_test.go
  • tests/cli_e2e/task/tasklist_add_task_workflow_test.go
  • tests/cli_e2e/task/tasklist_workflow_test.go

Comment thread tests/cli_e2e/base/helpers_test.go
@yxzhaao yxzhaao force-pushed the fix/fix-e2e-pagination branch 2 times, most recently from 77a1be9 to b522390 Compare April 23, 2026 13:42
@yxzhaao yxzhaao force-pushed the fix/fix-e2e-pagination branch from b522390 to b5251a9 Compare April 23, 2026 13:45
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.08%. Comparing base (10f1f2e) to head (b5251a9).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #639      +/-   ##
==========================================
+ Coverage   60.66%   61.08%   +0.42%     
==========================================
  Files         416      428      +12     
  Lines       43989    45190    +1201     
==========================================
+ Hits        26686    27605     +919     
- Misses      15233    15474     +241     
- Partials     2070     2111      +41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b5251a94e74fc59235e2f851eb27411970fbe9ca

🧩 Skill update

npx skills add yxzhaao/cli#fix/fix-e2e-pagination -y -g

@liangshuo-1 liangshuo-1 merged commit d3340f5 into larksuite:main Apr 23, 2026
16 checks passed
@liangshuo-1 liangshuo-1 mentioned this pull request Apr 24, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Low-risk docs, CI, test, or chore only changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants