Skip to content

fix(e2e): backfill created table fields#1386

Merged
liangshuo-1 merged 1 commit into
larksuite:mainfrom
yxzhaao:codex/base-table-create-fallback
Jun 10, 2026
Merged

fix(e2e): backfill created table fields#1386
liangshuo-1 merged 1 commit into
larksuite:mainfrom
yxzhaao:codex/base-table-create-fallback

Conversation

@yxzhaao

@yxzhaao yxzhaao commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • stop asserting primaryFieldID and primaryViewID in TestBase_BasicWorkflow
  • keep the workflow focused on the behaviors it actually validates: create/get/list table

Why

Those ids are not used anywhere else in the test flow. After the table-create refactor, whether data.fields is echoed in the create response can vary with backend behavior, which makes this live E2E assertion brittle without changing the workflow outcome.

Verification

  • go test ./tests/cli_e2e/base -run '^TestBase_BasicWorkflow$' -count=1
  • go test ./shortcuts/base -run '^TestBaseTableExecuteCreate$' -count=1

Summary by CodeRabbit

  • Tests
    • Updated internal test workflows to simplify validation logic.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR updates the TestBase_BasicWorkflow integration test to capture only the tableID return value from table creation, removing destructuring of unused primaryFieldID and primaryViewID identifiers and their corresponding empty-value assertions.

Changes

Integration Test Cleanup

Layer / File(s) Summary
Remove unused table creation return values and assertions
tests/cli_e2e/base/base_basic_workflow_test.go
The table setup call now captures only tableID from createTableWithRetry, discarding primaryFieldID and primaryViewID. The corresponding assertions validating those fields are non-empty are removed.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Suggested labels

domain/base, size/M

Suggested reviewers

  • zgz2048
  • kongenpei

Poem

A rabbit hops through test files bright, ✨
Removing unused values—what a sight!
tableID stands alone, clean and lean,
The workflow flows where checks have been. 🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description covers all required template sections: Summary explains the changes and rationale, Changes would be implicit from summary, Why provides context, and Verification lists test commands run.
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.
Title check ✅ Passed The title 'fix(e2e): backfill created table fields' directly describes the main change: dropping unused E2E assertions related to table field handling in the test file modified.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@github-actions github-actions Bot added domain/base PR touches the base domain size/M Single-domain feat or fix with limited business impact labels Jun 10, 2026
@yxzhaao
yxzhaao force-pushed the codex/base-table-create-fallback branch from 8140f13 to 26b5c7d Compare June 10, 2026 09:59
@yxzhaao yxzhaao changed the title fix(base): backfill created table fields test(base): remove brittle primary field/view assertions Jun 10, 2026
@yxzhaao yxzhaao changed the title test(base): remove brittle primary field/view assertions fix(e2e): backfill created table fields Jun 10, 2026
@github-actions github-actions Bot added size/S Low-risk docs, CI, test, or chore only changes and removed domain/base PR touches the base domain size/M Single-domain feat or fix with limited business impact labels Jun 10, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
shortcuts/base/table_ops.go (1)

121-127: 💤 Low value

Minor asymmetry in empty-fields handling.

When created["fields"] is present but empty ([]), line 115 assigns it to result["fields"]. When the fallback fetches zero fields, the len(fields) > 0 guard skips assignment, so result["fields"] remains unset. This creates asymmetry in the edge case where --fields is specified but the table ends up with zero fields.

Consider removing the len(fields) > 0 check and always assigning the fallback result to maintain consistency:

 		if len(fields) > 0 {
-			createdFields := make([]interface{}, 0, len(fields))
-			for _, field := range fields {
-				createdFields = append(createdFields, field)
-			}
-			result["fields"] = createdFields
+		createdFields := make([]interface{}, 0, len(fields))
+		for _, field := range fields {
+			createdFields = append(createdFields, field)
 		}
+		result["fields"] = createdFields

(Or keep the guard if the intent is to omit fields when the fallback returns an empty list.)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shortcuts/base/table_ops.go` around lines 121 - 127, The code handling
fallback field population is asymmetric: when created["fields"] exists but is an
empty slice you set result["fields"] earlier, but the fallback branch skips
assigning an empty [] because of the len(fields) > 0 guard; update the block in
the function that builds result (the code that iterates fields into
createdFields and assigns result["fields"]) to always assign result["fields"] =
createdFields (even when createdFields is empty) so created["fields"] and the
fallback behave consistently — remove the len(fields) > 0 check and always set
result["fields"].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@shortcuts/base/table_ops.go`:
- Around line 121-127: The code handling fallback field population is
asymmetric: when created["fields"] exists but is an empty slice you set
result["fields"] earlier, but the fallback branch skips assigning an empty []
because of the len(fields) > 0 guard; update the block in the function that
builds result (the code that iterates fields into createdFields and assigns
result["fields"]) to always assign result["fields"] = createdFields (even when
createdFields is empty) so created["fields"] and the fallback behave
consistently — remove the len(fields) > 0 check and always set result["fields"].

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7ca2fe34-7aec-4347-af84-e4f1239e1a76

📥 Commits

Reviewing files that changed from the base of the PR and between e751a53 and 8140f13.

📒 Files selected for processing (3)
  • shortcuts/base/base_execute_test.go
  • shortcuts/base/table_ops.go
  • tests/cli_e2e/base/base_basic_workflow_test.go

@github-actions

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

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

🧩 Skill update

npx skills add yxzhaao/cli#codex/base-table-create-fallback -y -g

@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.98%. Comparing base (e751a53) to head (26b5c7d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1386   +/-   ##
=======================================
  Coverage   71.98%   71.98%           
=======================================
  Files         700      700           
  Lines       66293    66293           
=======================================
  Hits        47718    47718           
  Misses      14870    14870           
  Partials     3705     3705           

☔ View full report in Codecov by Harness.
📢 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.

@liangshuo-1
liangshuo-1 merged commit 8e667db into larksuite:main Jun 10, 2026
20 checks passed
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