test: Add a prerequisite assumption verification test for correctness of sharedSchema#4252
test: Add a prerequisite assumption verification test for correctness of sharedSchema#4252hongyunyan wants to merge 4 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
the
release-note
Summary of ChangesHello @hongyunyan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the project's robustness by adding a critical test that safeguards against breaking changes in upstream TiDB's schema definitions. By proactively verifying the structure of key data types, it ensures the continued compatibility and correctness of TiCDC's shared-schema functionality, thereby reducing the risk of unexpected failures during TiDB version upgrades. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
ti-chi-bot
bot
added
the
size/XL
📝 WalkthroughWalkthroughAdds a new Go test that verifies upstream TiDB struct field contracts used by TiCDC's shared-schema logic by resolving module metadata, locating package sources, parsing Go AST (including embedded fields), and comparing extracted struct fields against expected lists. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test (pkg/common)
participant GoTool as Go Tooling
participant Upstream as Upstream Module (tidb)
participant Parser as AST Parser
participant Comparator as Field Comparator
Test->>GoTool: query module metadata (`go list -m -json`)
GoTool->>Upstream: download/locate module source if needed
Upstream-->>GoTool: provide source dir or zip
GoTool-->>Test: resolved package dir/zip
Test->>Parser: parse non-test .go files, collect struct fields (handle embedded)
Parser-->>Test: extracted struct field lists
Test->>Comparator: compare extracted vs expected fields
Comparator-->>Test: match / mismatch
alt mismatch
Test-->>Test: FailNow with diagnostics (module, version, hash, diffs)
else match
Test-->>Test: pass
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a new test file, table_info_shared_schema_guard_test.go, which is a valuable addition for verifying the contract of upstream TiDB structs. This guard test helps ensure compatibility and detect breaking changes early, improving the robustness of the shared-schema logic. The implementation is thorough, correctly handling module information retrieval, AST parsing for struct fields, and providing clear error messages upon contract mismatches. This is a well-designed and important test.
| @@ -0,0 +1,344 @@ | |||
| // Copyright 2026 PingCAP, Inc. | |||
There was a problem hiding this comment.
The copyright year is set to 2026. This appears to be a typo and should likely be the current year or the year the file was created.
| // Copyright 2026 PingCAP, Inc. | |
| // Copyright 2024 PingCAP, Inc. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/common/table_info_shared_schema_guard_test.go (3)
231-246: ParseDir filter accepts all files, then manually skips_test.go— consider filtering upfront.The
ParseDirfilter on line 233 returnstruefor every file, meaning test files are parsed into the AST and then skipped at line 244. Moving the test-file exclusion into the filter avoids unnecessary parsing work:♻️ Suggested filter change
- pkgs, err := parser.ParseDir(fileSet, packageDir, func(_ fs.FileInfo) bool { - return true - }, parser.SkipObjectResolution) + pkgs, err := parser.ParseDir(fileSet, packageDir, func(info fs.FileInfo) bool { + return !strings.HasSuffix(info.Name(), "_test.go") + }, parser.SkipObjectResolution)Then the manual check at lines 244–246 can be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 231 - 246, In extractStructFields update the parser.ParseDir file filter to return false for files with names ending in "_test.go" (e.g., check the fs.FileInfo.Name()) so test files are excluded during parsing, and then remove the subsequent manual skip that checks strings.HasSuffix(fileName, "_test.go") inside the package/files loop; this keeps the logic in the ParseDir filter and avoids parsing test files unnecessarily.
189-202:decodeModuleInfoJSON extraction is reasonable but has a subtle edge-case.
bytes.IndexByte(output, '{')finds the first{, which works well for typicalgo list/go mod downloadoutput. However,json.Unmarshal(output[start:])operates on everything from that first{to the end of the byte slice. If the command ever emits trailing output after the JSON (e.g., warnings on stderr mixed viaCombinedOutput), the unmarshal could fail.Using
json.NewDecoderon a reader fromoutput[start:]and callingDecodeonce would parse exactly one JSON object and ignore trailing bytes more robustly:💡 Optional: more robust JSON decoding
func decodeModuleInfo(output []byte) (moduleInfo, error) { start := bytes.IndexByte(output, '{') if start == -1 { return moduleInfo{}, fmt.Errorf("no JSON object found in output: %s", string(output)) } var mod moduleInfo - if err := json.Unmarshal(output[start:], &mod); err != nil { + if err := json.NewDecoder(bytes.NewReader(output[start:])).Decode(&mod); err != nil { return moduleInfo{}, err } return mod, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 189 - 202, The current decodeModuleInfo function finds the first '{' then calls json.Unmarshal on output[start:], which fails if there are trailing non-JSON bytes; change it to create an io.Reader from output[start:] (e.g., bytes.NewReader(output[start:])) and use json.NewDecoder(reader).Decode(&mod) to decode exactly one JSON object and ignore any trailing bytes, returning mod and any decode error; keep the function name decodeModuleInfo and variable mod the same so callers remain unchanged.
133-187:queryModuleInfocorrectly resolves and downloads module metadata.The fallback from
go listtogo mod downloadwhenDiris empty is a reasonable approach. The structured logging is compliant with project guidelines.One consideration: this function shells out to
go list -m -json <module>@master, which requires network access. If CI runs with restricted network orGOFLAGS=-mod=vendor, this test will fail. Consider gating behind a build tag (e.g.,//go:build integration) or an environment-variable check so it doesn't break offline/hermetic test runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 133 - 187, The test helper queryModuleInfo shells out to networked go commands and should be gated to avoid failing in offline/hermetic CI; modify the file so queryModuleInfo early-exits (call t.Skipf) when a guard is not set (either a build tag like //go:build integration at the top of the file or an environment check such as os.Getenv("RUN_INTEGRATION_TESTS") != "1"), or add the build tag to the file header and document usage; reference the queryModuleInfo function and use t.Skipf/t.Skip to skip the network-dependent behavior when the guard is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/common/table_info_shared_schema_guard_test.go`:
- Around line 231-246: In extractStructFields update the parser.ParseDir file
filter to return false for files with names ending in "_test.go" (e.g., check
the fs.FileInfo.Name()) so test files are excluded during parsing, and then
remove the subsequent manual skip that checks strings.HasSuffix(fileName,
"_test.go") inside the package/files loop; this keeps the logic in the ParseDir
filter and avoids parsing test files unnecessarily.
- Around line 189-202: The current decodeModuleInfo function finds the first '{'
then calls json.Unmarshal on output[start:], which fails if there are trailing
non-JSON bytes; change it to create an io.Reader from output[start:] (e.g.,
bytes.NewReader(output[start:])) and use json.NewDecoder(reader).Decode(&mod) to
decode exactly one JSON object and ignore any trailing bytes, returning mod and
any decode error; keep the function name decodeModuleInfo and variable mod the
same so callers remain unchanged.
- Around line 133-187: The test helper queryModuleInfo shells out to networked
go commands and should be gated to avoid failing in offline/hermetic CI; modify
the file so queryModuleInfo early-exits (call t.Skipf) when a guard is not set
(either a build tag like //go:build integration at the top of the file or an
environment check such as os.Getenv("RUN_INTEGRATION_TESTS") != "1"), or add the
build tag to the file header and document usage; reference the queryModuleInfo
function and use t.Skipf/t.Skip to skip the network-dependent behavior when the
guard is absent.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/common/table_info_shared_schema_guard_test.go`:
- Around line 57-139: The guard test TestLatestTiDBTableInfoSharedSchemaGuard
performs network-dependent go list/mod downloads against `@master` and must be
gated to keep default test runs deterministic; update the test to skip unless
explicitly enabled by (a) honoring testing.Short() (call t.Skip or t.Skipf when
testing.Short() is true) and (b) allowing an opt-in env var like
ENABLE_UPSTREAM_GUARD (check os.Getenv("ENABLE_UPSTREAM_GUARD") == "1" before
running) so the heavy logic that calls queryModuleInfo and
extractStructFieldsFromModule only runs when enabled; place the skip logic at
the start of TestLatestTiDBTableInfoSharedSchemaGuard and keep the rest of the
test (moduleCache, queryModuleInfo, assertStructContract) unchanged.
- Around line 203-214: The json.Unmarshal error returned from decodeModuleInfo
is unwrapped — change the return to wrap the library error using the project's
error helper (e.g., return moduleInfo{}, errors.Trace(err) or
errors.WrapError(err, "decodeModuleInfo") ) so the stack/context is preserved;
apply the same pattern to the other raw returns of json.Unmarshal and similar
library errors in this file (the other spots referenced in the review) so every
external error is wrapped at its origin before being returned.
| // TestLatestTiDBTableInfoSharedSchemaGuard verifies the upstream struct fields | ||
| // that TiCDC shared-schema logic depends on. | ||
| func TestLatestTiDBTableInfoSharedSchemaGuard(t *testing.T) { | ||
| // This guard intentionally checks the latest TiDB master to detect | ||
| // upstream struct-field changes before TiCDC upgrades its pinned TiDB version. | ||
| cases := []structGuardCase{ | ||
| { | ||
| name: "table_info_fields_for_shared_schema", | ||
| modulePath: "github.com/pingcap/tidb", | ||
| pkgRelativePath: "pkg/meta/model", | ||
| typeName: "TableInfo", | ||
| expectedFields: []string{ | ||
| "ID", "Name", "Charset", "Collate", "Columns", "Indices", "Constraints", "ForeignKeys", "State", | ||
| "PKIsHandle", "IsCommonHandle", "CommonHandleVersion", | ||
| "Comment", "AutoIncID", "AutoIncIDExtra", "AutoIDCache", "AutoRandID", | ||
| "MaxColumnID", "MaxIndexID", "MaxForeignKeyID", "MaxConstraintID", "UpdateTS", "AutoIDSchemaID", | ||
| "ShardRowIDBits", "MaxShardRowIDBits", "AutoRandomBits", "AutoRandomRangeBits", "PreSplitRegions", | ||
| "Partition", "Compression", "View", "Sequence", "Lock", "Version", "TiFlashReplica", "IsColumnar", | ||
| "TempTableType", "TableCacheStatusType", "PlacementPolicyRef", "StatsOptions", | ||
| "ExchangePartitionInfo", "TTLInfo", "IsActiveActive", "SoftdeleteInfo", "Affinity", | ||
| "Revision", "DBID", "Mode", | ||
| }, | ||
| }, | ||
| { | ||
| name: "column_info_fields_for_shared_schema", | ||
| modulePath: "github.com/pingcap/tidb", | ||
| pkgRelativePath: "pkg/meta/model", | ||
| typeName: "ColumnInfo", | ||
| expectedFields: []string{ | ||
| "ID", "Name", "Offset", "OriginDefaultValue", "OriginDefaultValueBit", "DefaultValue", "DefaultValueBit", | ||
| "DefaultIsExpr", "GeneratedExprString", "GeneratedStored", "Dependences", "FieldType", "ChangingFieldType", | ||
| "State", "Comment", "Hidden", "ChangeStateInfo", "Version", | ||
| }, | ||
| }, | ||
| { | ||
| name: "index_info_fields_for_shared_schema", | ||
| modulePath: "github.com/pingcap/tidb", | ||
| pkgRelativePath: "pkg/meta/model", | ||
| typeName: "IndexInfo", | ||
| expectedFields: []string{ | ||
| "ID", "Name", "Table", "Columns", "State", "BackfillState", "Comment", "Tp", "Unique", "Primary", | ||
| "Invisible", "Global", "MVIndex", "VectorInfo", "InvertedInfo", "FullTextInfo", | ||
| "ConditionExprString", "AffectColumn", "GlobalIndexVersion", | ||
| }, | ||
| }, | ||
| { | ||
| name: "index_column_fields_for_shared_schema", | ||
| modulePath: "github.com/pingcap/tidb", | ||
| pkgRelativePath: "pkg/meta/model", | ||
| typeName: "IndexColumn", | ||
| expectedFields: []string{"Name", "Offset", "Length", "UseChangingType"}, | ||
| }, | ||
| { | ||
| name: "field_type_fields_for_shared_schema", | ||
| modulePath: "github.com/pingcap/tidb/pkg/parser", | ||
| pkgRelativePath: "types", | ||
| typeName: "FieldType", | ||
| expectedFields: []string{"tp", "flag", "flen", "decimal", "charset", "collate", "elems", "elemsIsBinaryLit", "array"}, | ||
| }, | ||
| } | ||
|
|
||
| moduleCache := make(map[string]*moduleInfo) | ||
| for _, tc := range cases { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| mod, ok := moduleCache[tc.modulePath] | ||
| if !ok { | ||
| mod = queryModuleInfo(t, tc.modulePath) | ||
| moduleCache[tc.modulePath] = mod | ||
| } | ||
|
|
||
| actualFields, err := extractStructFieldsFromModule(mod, tc.pkgRelativePath, tc.typeName) | ||
| require.NoError( | ||
| t, | ||
| err, | ||
| "extract struct fields failed for %s in module=%s version=%s", | ||
| tc.typeName, | ||
| mod.Path, | ||
| mod.Version, | ||
| ) | ||
|
|
||
| assertStructContract(t, tc, mod, actualFields) | ||
| }) |
There was a problem hiding this comment.
Gate the guard test to keep default runs deterministic.
This test shells out to go list/go mod download against @master, so it depends on network access and mutable upstream state. That makes normal unit test runs flaky. Consider skipping unless explicitly enabled (env var/build tag), and honor -short.
Suggested guard to keep default test runs deterministic
@@
"io"
"io/fs"
+ "os"
"os/exec"
@@
func TestLatestTiDBTableInfoSharedSchemaGuard(t *testing.T) {
+ if testing.Short() || os.Getenv("TICDC_SHARED_SCHEMA_GUARD") == "" {
+ t.Skip("shared schema guard requires network; set TICDC_SHARED_SCHEMA_GUARD=1 to run")
+ }
// This guard intentionally checks the latest TiDB master to detectAs per coding guidelines **/*_test.go: favor deterministic tests and use testify/require.
📝 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.
| // TestLatestTiDBTableInfoSharedSchemaGuard verifies the upstream struct fields | |
| // that TiCDC shared-schema logic depends on. | |
| func TestLatestTiDBTableInfoSharedSchemaGuard(t *testing.T) { | |
| // This guard intentionally checks the latest TiDB master to detect | |
| // upstream struct-field changes before TiCDC upgrades its pinned TiDB version. | |
| cases := []structGuardCase{ | |
| { | |
| name: "table_info_fields_for_shared_schema", | |
| modulePath: "github.com/pingcap/tidb", | |
| pkgRelativePath: "pkg/meta/model", | |
| typeName: "TableInfo", | |
| expectedFields: []string{ | |
| "ID", "Name", "Charset", "Collate", "Columns", "Indices", "Constraints", "ForeignKeys", "State", | |
| "PKIsHandle", "IsCommonHandle", "CommonHandleVersion", | |
| "Comment", "AutoIncID", "AutoIncIDExtra", "AutoIDCache", "AutoRandID", | |
| "MaxColumnID", "MaxIndexID", "MaxForeignKeyID", "MaxConstraintID", "UpdateTS", "AutoIDSchemaID", | |
| "ShardRowIDBits", "MaxShardRowIDBits", "AutoRandomBits", "AutoRandomRangeBits", "PreSplitRegions", | |
| "Partition", "Compression", "View", "Sequence", "Lock", "Version", "TiFlashReplica", "IsColumnar", | |
| "TempTableType", "TableCacheStatusType", "PlacementPolicyRef", "StatsOptions", | |
| "ExchangePartitionInfo", "TTLInfo", "IsActiveActive", "SoftdeleteInfo", "Affinity", | |
| "Revision", "DBID", "Mode", | |
| }, | |
| }, | |
| { | |
| name: "column_info_fields_for_shared_schema", | |
| modulePath: "github.com/pingcap/tidb", | |
| pkgRelativePath: "pkg/meta/model", | |
| typeName: "ColumnInfo", | |
| expectedFields: []string{ | |
| "ID", "Name", "Offset", "OriginDefaultValue", "OriginDefaultValueBit", "DefaultValue", "DefaultValueBit", | |
| "DefaultIsExpr", "GeneratedExprString", "GeneratedStored", "Dependences", "FieldType", "ChangingFieldType", | |
| "State", "Comment", "Hidden", "ChangeStateInfo", "Version", | |
| }, | |
| }, | |
| { | |
| name: "index_info_fields_for_shared_schema", | |
| modulePath: "github.com/pingcap/tidb", | |
| pkgRelativePath: "pkg/meta/model", | |
| typeName: "IndexInfo", | |
| expectedFields: []string{ | |
| "ID", "Name", "Table", "Columns", "State", "BackfillState", "Comment", "Tp", "Unique", "Primary", | |
| "Invisible", "Global", "MVIndex", "VectorInfo", "InvertedInfo", "FullTextInfo", | |
| "ConditionExprString", "AffectColumn", "GlobalIndexVersion", | |
| }, | |
| }, | |
| { | |
| name: "index_column_fields_for_shared_schema", | |
| modulePath: "github.com/pingcap/tidb", | |
| pkgRelativePath: "pkg/meta/model", | |
| typeName: "IndexColumn", | |
| expectedFields: []string{"Name", "Offset", "Length", "UseChangingType"}, | |
| }, | |
| { | |
| name: "field_type_fields_for_shared_schema", | |
| modulePath: "github.com/pingcap/tidb/pkg/parser", | |
| pkgRelativePath: "types", | |
| typeName: "FieldType", | |
| expectedFields: []string{"tp", "flag", "flen", "decimal", "charset", "collate", "elems", "elemsIsBinaryLit", "array"}, | |
| }, | |
| } | |
| moduleCache := make(map[string]*moduleInfo) | |
| for _, tc := range cases { | |
| tc := tc | |
| t.Run(tc.name, func(t *testing.T) { | |
| mod, ok := moduleCache[tc.modulePath] | |
| if !ok { | |
| mod = queryModuleInfo(t, tc.modulePath) | |
| moduleCache[tc.modulePath] = mod | |
| } | |
| actualFields, err := extractStructFieldsFromModule(mod, tc.pkgRelativePath, tc.typeName) | |
| require.NoError( | |
| t, | |
| err, | |
| "extract struct fields failed for %s in module=%s version=%s", | |
| tc.typeName, | |
| mod.Path, | |
| mod.Version, | |
| ) | |
| assertStructContract(t, tc, mod, actualFields) | |
| }) | |
| // TestLatestTiDBTableInfoSharedSchemaGuard verifies the upstream struct fields | |
| // that TiCDC shared-schema logic depends on. | |
| func TestLatestTiDBTableInfoSharedSchemaGuard(t *testing.T) { | |
| if testing.Short() || os.Getenv("TICDC_SHARED_SCHEMA_GUARD") == "" { | |
| t.Skip("shared schema guard requires network; set TICDC_SHARED_SCHEMA_GUARD=1 to run") | |
| } | |
| // This guard intentionally checks the latest TiDB master to detect | |
| // upstream struct-field changes before TiCDC upgrades its pinned TiDB version. | |
| cases := []structGuardCase{ | |
| { | |
| name: "table_info_fields_for_shared_schema", | |
| modulePath: "github.com/pingcap/tidb", | |
| pkgRelativePath: "pkg/meta/model", | |
| typeName: "TableInfo", | |
| expectedFields: []string{ | |
| "ID", "Name", "Charset", "Collate", "Columns", "Indices", "Constraints", "ForeignKeys", "State", | |
| "PKIsHandle", "IsCommonHandle", "CommonHandleVersion", | |
| "Comment", "AutoIncID", "AutoIncIDExtra", "AutoIDCache", "AutoRandID", | |
| "MaxColumnID", "MaxIndexID", "MaxForeignKeyID", "MaxConstraintID", "UpdateTS", "AutoIDSchemaID", | |
| "ShardRowIDBits", "MaxShardRowIDBits", "AutoRandomBits", "AutoRandomRangeBits", "PreSplitRegions", | |
| "Partition", "Compression", "View", "Sequence", "Lock", "Version", "TiFlashReplica", "IsColumnar", | |
| "TempTableType", "TableCacheStatusType", "PlacementPolicyRef", "StatsOptions", | |
| "ExchangePartitionInfo", "TTLInfo", "IsActiveActive", "SoftdeleteInfo", "Affinity", | |
| "Revision", "DBID", "Mode", | |
| }, | |
| }, | |
| { | |
| name: "column_info_fields_for_shared_schema", | |
| modulePath: "github.com/pingcap/tidb", | |
| pkgRelativePath: "pkg/meta/model", | |
| typeName: "ColumnInfo", | |
| expectedFields: []string{ | |
| "ID", "Name", "Offset", "OriginDefaultValue", "OriginDefaultValueBit", "DefaultValue", "DefaultValueBit", | |
| "DefaultIsExpr", "GeneratedExprString", "GeneratedStored", "Dependences", "FieldType", "ChangingFieldType", | |
| "State", "Comment", "Hidden", "ChangeStateInfo", "Version", | |
| }, | |
| }, | |
| { | |
| name: "index_info_fields_for_shared_schema", | |
| modulePath: "github.com/pingcap/tidb", | |
| pkgRelativePath: "pkg/meta/model", | |
| typeName: "IndexInfo", | |
| expectedFields: []string{ | |
| "ID", "Name", "Table", "Columns", "State", "BackfillState", "Comment", "Tp", "Unique", "Primary", | |
| "Invisible", "Global", "MVIndex", "VectorInfo", "InvertedInfo", "FullTextInfo", | |
| "ConditionExprString", "AffectColumn", "GlobalIndexVersion", | |
| }, | |
| }, | |
| { | |
| name: "index_column_fields_for_shared_schema", | |
| modulePath: "github.com/pingcap/tidb", | |
| pkgRelativePath: "pkg/meta/model", | |
| typeName: "IndexColumn", | |
| expectedFields: []string{"Name", "Offset", "Length", "UseChangingType"}, | |
| }, | |
| { | |
| name: "field_type_fields_for_shared_schema", | |
| modulePath: "github.com/pingcap/tidb/pkg/parser", | |
| pkgRelativePath: "types", | |
| typeName: "FieldType", | |
| expectedFields: []string{"tp", "flag", "flen", "decimal", "charset", "collate", "elems", "elemsIsBinaryLit", "array"}, | |
| }, | |
| } | |
| moduleCache := make(map[string]*moduleInfo) | |
| for _, tc := range cases { | |
| tc := tc | |
| t.Run(tc.name, func(t *testing.T) { | |
| mod, ok := moduleCache[tc.modulePath] | |
| if !ok { | |
| mod = queryModuleInfo(t, tc.modulePath) | |
| moduleCache[tc.modulePath] = mod | |
| } | |
| actualFields, err := extractStructFieldsFromModule(mod, tc.pkgRelativePath, tc.typeName) | |
| require.NoError( | |
| t, | |
| err, | |
| "extract struct fields failed for %s in module=%s version=%s", | |
| tc.typeName, | |
| mod.Path, | |
| mod.Version, | |
| ) | |
| assertStructContract(t, tc, mod, actualFields) | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 57 - 139, The
guard test TestLatestTiDBTableInfoSharedSchemaGuard performs network-dependent
go list/mod downloads against `@master` and must be gated to keep default test
runs deterministic; update the test to skip unless explicitly enabled by (a)
honoring testing.Short() (call t.Skip or t.Skipf when testing.Short() is true)
and (b) allowing an opt-in env var like ENABLE_UPSTREAM_GUARD (check
os.Getenv("ENABLE_UPSTREAM_GUARD") == "1" before running) so the heavy logic
that calls queryModuleInfo and extractStructFieldsFromModule only runs when
enabled; place the skip logic at the start of
TestLatestTiDBTableInfoSharedSchemaGuard and keep the rest of the test
(moduleCache, queryModuleInfo, assertStructContract) unchanged.
| // decodeModuleInfo parses module metadata JSON from go command output. | ||
| // Some go commands may print informational lines before JSON payload. | ||
| func decodeModuleInfo(output []byte) (moduleInfo, error) { | ||
| start := bytes.IndexByte(output, '{') | ||
| if start == -1 { | ||
| return moduleInfo{}, fmt.Errorf("no JSON object found in output: %s", string(output)) | ||
| } | ||
|
|
||
| var mod moduleInfo | ||
| if err := json.Unmarshal(output[start:], &mod); err != nil { | ||
| return moduleInfo{}, err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the specific lines
find . -name "table_info_shared_schema_guard_test.go" -type fRepository: pingcap/ticdc
Length of output: 108
🏁 Script executed:
# Once found, let's read the file around lines 203-214
# First get the total line count
wc -l ./pkg/common/table_info_shared_schema_guard_test.goRepository: pingcap/ticdc
Length of output: 112
🏁 Script executed:
# Read lines 200-220 to see the context around lines 203-214
sed -n '200,220p' ./pkg/common/table_info_shared_schema_guard_test.goRepository: pingcap/ticdc
Length of output: 827
🏁 Script executed:
# Check the imports at the top of the file to see if errors is imported
head -50 ./pkg/common/table_info_shared_schema_guard_test.goRepository: pingcap/ticdc
Length of output: 1206
🏁 Script executed:
# Search for other unwrapped error returns in this file
grep -n "return.*err" ./pkg/common/table_info_shared_schema_guard_test.goRepository: pingcap/ticdc
Length of output: 224
Wrap library errors at the origin.
decodeModuleInfo returns raw json.Unmarshal errors without wrapping, which loses stack context required by the repo's error-handling guidelines. Wrap with errors.Trace() or errors.WrapError() before returning.
Example fix for decodeModuleInfo
"github.com/pingcap/log"
+ "github.com/pingcap/errors"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
if err := json.Unmarshal(output[start:], &mod); err != nil {
- return moduleInfo{}, err
+ return moduleInfo{}, errors.Trace(err)
}Note: Similar unwrapped errors appear on lines 260, 293, 306, 309, 333, and 344—apply the same wrapping pattern throughout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 203 - 214,
The json.Unmarshal error returned from decodeModuleInfo is unwrapped — change
the return to wrap the library error using the project's error helper (e.g.,
return moduleInfo{}, errors.Trace(err) or errors.WrapError(err,
"decodeModuleInfo") ) so the stack/context is preserved; apply the same pattern
to the other raw returns of json.Unmarshal and similar library errors in this
file (the other spots referenced in the review) so every external error is
wrapped at its origin before being returned.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/common/table_info_shared_schema_guard_test.go (2)
203-213: [Unresolved] Third-party errors returned unwrapped throughout the file.
json.Unmarshal(line 213),zip.OpenReader(line 260),file.Open()(line 294),io.ReadAll(line 299),rc.Close()(line 302), andparser.ParseFile(line 307) allreturn nil, errwithouterrors.Trace(err), losing stack context required by the repo's error-handling convention. The past review identified all these locations — none have been addressed.As per coding guidelines
**/*.go: wrap third-party library errors immediately witherrors.Trace(err).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 203 - 213, Multiple third-party errors in this file are returned directly instead of being wrapped with errors.Trace, losing stack context; update all return sites in functions like decodeModuleInfo (json.Unmarshal), and the functions that call zip.OpenReader, file.Open, io.ReadAll, rc.Close, and parser.ParseFile to wrap the underlying error with errors.Trace(err) before returning (e.g., return nil, errors.Trace(err)) so they follow the repo convention and preserve stack traces.
59-141: [Unresolved] Gate the test behind an opt-in mechanism to keep default runs deterministic.The test still shells out to
go list@master`` /go mod downloadon every `go test ./...` invocation, making it network-dependent and non-deterministic. The past review requested a `testing.Short()` guard and an environment-variable opt-in, neither of which has been applied.As per coding guidelines
**/*_test.go: favor deterministic tests and usetestify/require.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 59 - 141, Gate TestLatestTiDBTableInfoSharedSchemaGuard so it doesn't run network calls by default: at the top of the function check testing.Short() and an opt-in env var (e.g. os.Getenv("RUN_LATEST_TIDB_GUARD") == "1") and call t.Skip when not enabled, before any calls to queryModuleInfo or extractStructFieldsFromModule; ensure os and testing are imported and use testify/require for assertions as already used.
🧹 Nitpick comments (2)
pkg/common/table_info_shared_schema_guard_test.go (2)
330-343:_test.gofiles are parsed unnecessarily — filter them in theParseDircallback.The filter function always returns
true, soParseDirparses every file including_test.go, which the loop then discards. Move the exclusion into the filter to avoid the redundant AST work.♻️ Proposed refactor
- pkgs, err := parser.ParseDir(fileSet, packageDir, func(_ fs.FileInfo) bool { - return true - }, parser.SkipObjectResolution) + pkgs, err := parser.ParseDir(fileSet, packageDir, func(fi fs.FileInfo) bool { + return !strings.HasSuffix(fi.Name(), "_test.go") + }, parser.SkipObjectResolution) if err != nil { return nil, err } fieldSet := make(map[string]struct{}) found := false for _, pkg := range pkgs { for fileName, fileNode := range pkg.Files { - if strings.HasSuffix(fileName, "_test.go") { - continue - } if err := collectStructFieldsFromFile(fileNode, typeName, fieldSet, &found); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 330 - 343, The ParseDir call currently passes a callback that always returns true causing parser.ParseDir to include _test.go files only to have them filtered later; update the anonymous filter callback passed to parser.ParseDir (the func(_ fs.FileInfo) bool {...}) so it returns false for files with names ending in "_test.go" (e.g. use fs.FileInfo.Name() and strings.HasSuffix) and true otherwise, keeping the existing parser.SkipObjectResolution option; this moves the exclusion into the ParseDir scan and avoids building ASTs for test files referenced by fileSet/packageDir.
279-287: Substring match for zip entry path could falsely include files from sibling directories.
strings.Index(name, "/"+targetDir)will match any occurrence of the suffix in the path, e.g. a hypotheticalinternal/pkg/meta/model/foo.gowould also match/pkg/meta/model/. Module zip entries are guaranteed to start with<module>@<version>/, so it is safer to strip that fixed prefix before comparing the relative path.♻️ More precise path matching
- targetPrefix := "/" + targetDir - pkgPos := strings.Index(name, targetPrefix) - if pkgPos < 0 { - continue - } - rest := name[pkgPos+len(targetPrefix):] + // Module zip entries are "<module>@<version>/rel/path". Strip the root prefix. + atIdx := strings.Index(name, "@") + if atIdx < 0 { + continue + } + slashIdx := strings.Index(name[atIdx:], "/") + if slashIdx < 0 { + continue + } + relPath := name[atIdx+slashIdx+1:] // "rel/path/file.go" + if !strings.HasPrefix(relPath, targetDir) { + continue + } + rest := relPath[len(targetDir):]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 279 - 287, The current substring check using strings.Index(name, "/"+targetDir) can match occurrences inside deeper paths; instead strip the leading module@version prefix from the zip entry (use the first '/' in name) to get the true relative path, then compare that relative path against targetDir; update the logic that computes pkgPos/rest/targetPrefix (variables name, targetPrefix, pkgPos, rest, targetDir) so you derive rel := name[firstSlash+1:] and then check rel for the exact sibling directory (e.g., ensure rel has no extra '/' after the targetDir) rather than searching the whole name for "/"+targetDir.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/common/table_info_shared_schema_guard_test.go`:
- Around line 148-164: The test uses exec.Command for external calls (the "go
list" invocation that constructs cmd := exec.Command(...) and the "go mod
download" invocation that constructs downloadCmd := exec.Command(...)) with no
timeout; change both to use exec.CommandContext with a context that has a
bounded timeout (use context.WithTimeout to create ctx and defer cancel) so the
test cannot hang indefinitely, and pass ctx into exec.CommandContext for the "go
list" and "go mod download" commands (keep existing output/error handling and
decodeModuleInfo usage unchanged).
---
Duplicate comments:
In `@pkg/common/table_info_shared_schema_guard_test.go`:
- Around line 203-213: Multiple third-party errors in this file are returned
directly instead of being wrapped with errors.Trace, losing stack context;
update all return sites in functions like decodeModuleInfo (json.Unmarshal), and
the functions that call zip.OpenReader, file.Open, io.ReadAll, rc.Close, and
parser.ParseFile to wrap the underlying error with errors.Trace(err) before
returning (e.g., return nil, errors.Trace(err)) so they follow the repo
convention and preserve stack traces.
- Around line 59-141: Gate TestLatestTiDBTableInfoSharedSchemaGuard so it
doesn't run network calls by default: at the top of the function check
testing.Short() and an opt-in env var (e.g. os.Getenv("RUN_LATEST_TIDB_GUARD")
== "1") and call t.Skip when not enabled, before any calls to queryModuleInfo or
extractStructFieldsFromModule; ensure os and testing are imported and use
testify/require for assertions as already used.
---
Nitpick comments:
In `@pkg/common/table_info_shared_schema_guard_test.go`:
- Around line 330-343: The ParseDir call currently passes a callback that always
returns true causing parser.ParseDir to include _test.go files only to have them
filtered later; update the anonymous filter callback passed to parser.ParseDir
(the func(_ fs.FileInfo) bool {...}) so it returns false for files with names
ending in "_test.go" (e.g. use fs.FileInfo.Name() and strings.HasSuffix) and
true otherwise, keeping the existing parser.SkipObjectResolution option; this
moves the exclusion into the ParseDir scan and avoids building ASTs for test
files referenced by fileSet/packageDir.
- Around line 279-287: The current substring check using strings.Index(name,
"/"+targetDir) can match occurrences inside deeper paths; instead strip the
leading module@version prefix from the zip entry (use the first '/' in name) to
get the true relative path, then compare that relative path against targetDir;
update the logic that computes pkgPos/rest/targetPrefix (variables name,
targetPrefix, pkgPos, rest, targetDir) so you derive rel := name[firstSlash+1:]
and then check rel for the exact sibling directory (e.g., ensure rel has no
extra '/' after the targetDir) rather than searching the whole name for
"/"+targetDir.
| cmd := exec.Command("go", "list", "-m", "-json", modulePath+"@master") | ||
| output, err := cmd.CombinedOutput() | ||
| require.NoError(t, err, "query module %s failed: %s", modulePath, string(output)) | ||
|
|
||
| mod, err := decodeModuleInfo(output) | ||
| require.NoError(t, err, "unmarshal module metadata for %s failed", modulePath) | ||
|
|
||
| // go list may omit Dir when the queried version is not yet downloaded. | ||
| // In that case, explicitly download the resolved version to get a stable | ||
| // source directory for AST parsing. | ||
| if mod.Dir == "" { | ||
| query := modulePath + "@master" | ||
| if mod.Version != "" { | ||
| query = modulePath + "@" + mod.Version | ||
| } | ||
| downloadCmd := exec.Command("go", "mod", "download", "-json", query) | ||
| downloadOutput, downloadErr := downloadCmd.CombinedOutput() |
There was a problem hiding this comment.
exec.Command calls have no timeout — CI jobs can hang indefinitely.
Both go list (line 148) and go mod download (line 163) have no context deadline. A slow or unreachable module proxy will block the test runner forever. Use exec.CommandContext with a bounded timeout.
⏱️ Proposed fix using exec.CommandContext
+import (
+ "context"
+ "time"
+ // … existing imports …
+)
func queryModuleInfo(t *testing.T, modulePath string) *moduleInfo {
t.Helper()
- cmd := exec.Command("go", "list", "-m", "-json", modulePath+"@master")
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+ defer cancel()
+ cmd := exec.CommandContext(ctx, "go", "list", "-m", "-json", modulePath+"@master")
output, err := cmd.CombinedOutput()
require.NoError(t, err, "query module %s failed: %s", modulePath, string(output))Apply the same ctx / exec.CommandContext wrapping to the go mod download call on line 163.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 148 - 164,
The test uses exec.Command for external calls (the "go list" invocation that
constructs cmd := exec.Command(...) and the "go mod download" invocation that
constructs downloadCmd := exec.Command(...)) with no timeout; change both to use
exec.CommandContext with a context that has a bounded timeout (use
context.WithTimeout to create ctx and defer cancel) so the test cannot hang
indefinitely, and pass ctx into exec.CommandContext for the "go list" and "go
mod download" commands (keep existing output/error handling and decodeModuleInfo
usage unchanged).
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pkg/common/table_info_shared_schema_guard_test.go (3)
67-68:⚠️ Potential issue | 🟠 MajorGate this test behind an opt-in mechanism to preserve CI determinism.
This test shells out to
go mod download …@master``, which requires network access and depends on mutable upstream state. Running it unconditionally in everygo testinvocation makes unit test suites non-deterministic and flaky. Add a skip guard at the top of the function.Proposed skip guard
func TestLatestTiDBTableInfoSharedSchemaGuard(t *testing.T) { + if testing.Short() || os.Getenv("TICDC_SHARED_SCHEMA_GUARD") == "" { + t.Skip("shared-schema guard requires network; set TICDC_SHARED_SCHEMA_GUARD=1 to run") + } // This guard intentionally checks the latest TiDB master to detectAs per coding guidelines
**/*_test.go: favor deterministic tests and usetestify/require.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 67 - 68, The test TestLatestTiDBTableInfoSharedSchemaGuard performs networked, non-deterministic work and must be opt-in: at the top of the function add a skip guard that only runs when an explicit opt-in flag is set (for example check an env var like RUN_NETWORK_TESTS=="1" or a build tag) and call t.Skip with a clear message otherwise; also ensure the test imports and uses testify/require where applicable per project guidelines.
210-216:⚠️ Potential issue | 🟠 Major
exec.Commandhas no timeout — CI jobs can hang indefinitely.
go mod downloadagainst@masterthrough a potentially unreachable proxy has no deadline. Useexec.CommandContextwith a bounded timeout so the test fails cleanly instead of blocking the runner.Proposed fix
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() - cmd := exec.Command("go", "mod", "download", "-json", modulePath+"@master") + cmd := exec.CommandContext(ctx, "go", "mod", "download", "-json", modulePath+"@master")Requires adding
"context"and"time"to imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 210 - 216, The test uses exec.Command("go", "mod", "download", ...) with no timeout which can hang CI; change to use context.WithTimeout and exec.CommandContext so the subprocess is bounded. Replace the exec.Command call with a context.WithTimeout (e.g. ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second); defer cancel()) and create the command with exec.CommandContext(ctx, "go", "mod", "download", "-json", modulePath+"@master"), then call cmd.CombinedOutput() as before; also add the "context" and "time" imports. Ensure you keep existing error handling around decodeModuleInfo and the require.NoError checks.
234-236: Unwrapped library error fromjson.Unmarshal.Per the repo's error-handling guidelines, third-party/library errors should be wrapped with
errors.Trace(err)to preserve stack context. The same applies to other rawreturn nil, errsites in this file (lines 293, 309, 322, 354, 359, 362, 387).Example fix
if err := json.Unmarshal(output[start:], &mod); err != nil { - return moduleInfo{}, err + return moduleInfo{}, errors.Trace(err) }Add
"github.com/pingcap/errors"to imports and apply the same pattern to all other raw library error returns.Based on learnings: "When an error comes from a third-party or library call in Go, wrap it immediately with
errors.Trace(err)orerrors.WrapError(...)to attach a stack trace."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 234 - 236, Wrap raw library errors returned from calls like json.Unmarshal(output[start:], &mod) with errors.Trace(err) and add "github.com/pingcap/errors" to the imports; specifically replace the raw return of moduleInfo{}, err with moduleInfo{}, errors.Trace(err) for the json.Unmarshal call (and apply the same pattern for other raw returns that return nil, err in this file). Update all other occurrences mentioned (the other raw library returns in this file) to use errors.Trace(err) so third-party errors preserve stack context; keep existing return types (e.g., moduleInfo{}, nil) unchanged except for wrapping the error value.
🧹 Nitpick comments (3)
pkg/common/table_info_shared_schema_guard_test.go (3)
384-388: Build-tagged source files are parsed unconditionally.
parser.ParseFiledoes not evaluate//go:buildconstraints. If an upstream package ever defines the same struct in platform-specific files (e.g.foo_linux.go/foo_windows.go), the "type found multiple times" guard on line 444 would trigger a spurious failure. For TiDB model types this is currently fine, but consider filtering files withgo/build.Context.MatchFileor at least documenting this assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 384 - 388, The test parses all files in sourceFiles with parser.ParseFile (using fileSet and parser.SkipObjectResolution) without applying build-tag constraints, which can cause duplicate-type errors when upstream packages have platform-specific files; update the loop that calls parser.ParseFile to first filter sourceFiles by evaluating build tags (e.g. use go/build.Context.MatchFile or equivalent) and only parse files that match the current build context (or add a clear comment documenting the assumption), so that parser.ParseFile is invoked only for files valid for the active platform.
338-347: Zip entry path matching uses substring search — may false-match nested directories.
strings.Index(name, targetPrefix)finds the first occurrence of"/types/"(or whateverpkgRelativePathresolves to) anywhere in the zip entry path. If the module ever contains multiple directories ending with the same suffix (e.g.internal/types/alongsidetypes/), this could match the wrong package.A safer approach is to strip the known
<module>@<version>/prefix first and then compare the remaining directory exactly:Suggested approach
for _, file := range reader.File { ... name := filepath.ToSlash(file.Name) - targetPrefix := "/" + targetDir - pkgPos := strings.Index(name, targetPrefix) - if pkgPos < 0 { + // Strip the first path component "<module>@<version>/" + slashIdx := strings.Index(name, "/") + if slashIdx < 0 { continue } - rest := name[pkgPos+len(targetPrefix):] + relPath := name[slashIdx+1:] + if !strings.HasPrefix(relPath, targetDir) { + continue + } + rest := relPath[len(targetDir):]This is low-risk for the known TiDB modules today, but the exact-prefix approach is more robust if the upstream module structure evolves.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 338 - 347, The code uses strings.Index(name, targetPrefix) (where targetPrefix := "/" + targetDir) which can match the suffix anywhere in the zip entry path; instead strip the leading module@version component first and then do an exact segment match: find the first '/' in name, set rest to the substring after that (replace current pkgPos/rest logic), then check if rest equals targetDir or starts with targetDir+"/" (using rest == targetDir || strings.HasPrefix(rest, targetDir+"/")) so you only match the top-level package directory; update references to name, targetDir, targetPrefix, pkgPos and rest accordingly in this block.
129-131: Remove the unnecessarytc := tcre-binding; loop variables are per-iteration scoped in Go 1.22+.The project targets Go 1.25.5, which makes the
tc := tcpattern a no-op. This re-binding was a workaround for shared loop variable scoping in older Go versions but is unnecessary now and can be safely removed for cleaner code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info_shared_schema_guard_test.go` around lines 129 - 131, Remove the redundant loop-variable re-binding inside the test loop: in the for-range over cases, delete the unnecessary "tc := tc" statement so each iteration uses the per-iteration scoped loop variable "tc" directly when calling t.Run and within the subtest; update the test body that references tc (the subtest closure passed to t.Run) to rely on the loop's per-iteration scoping and ensure no other re-bindings remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/common/table_info_shared_schema_guard_test.go`:
- Around line 67-68: The test TestLatestTiDBTableInfoSharedSchemaGuard performs
networked, non-deterministic work and must be opt-in: at the top of the function
add a skip guard that only runs when an explicit opt-in flag is set (for example
check an env var like RUN_NETWORK_TESTS=="1" or a build tag) and call t.Skip
with a clear message otherwise; also ensure the test imports and uses
testify/require where applicable per project guidelines.
- Around line 210-216: The test uses exec.Command("go", "mod", "download", ...)
with no timeout which can hang CI; change to use context.WithTimeout and
exec.CommandContext so the subprocess is bounded. Replace the exec.Command call
with a context.WithTimeout (e.g. ctx, cancel :=
context.WithTimeout(context.Background(), 30*time.Second); defer cancel()) and
create the command with exec.CommandContext(ctx, "go", "mod", "download",
"-json", modulePath+"@master"), then call cmd.CombinedOutput() as before; also
add the "context" and "time" imports. Ensure you keep existing error handling
around decodeModuleInfo and the require.NoError checks.
- Around line 234-236: Wrap raw library errors returned from calls like
json.Unmarshal(output[start:], &mod) with errors.Trace(err) and add
"github.com/pingcap/errors" to the imports; specifically replace the raw return
of moduleInfo{}, err with moduleInfo{}, errors.Trace(err) for the json.Unmarshal
call (and apply the same pattern for other raw returns that return nil, err in
this file). Update all other occurrences mentioned (the other raw library
returns in this file) to use errors.Trace(err) so third-party errors preserve
stack context; keep existing return types (e.g., moduleInfo{}, nil) unchanged
except for wrapping the error value.
---
Nitpick comments:
In `@pkg/common/table_info_shared_schema_guard_test.go`:
- Around line 384-388: The test parses all files in sourceFiles with
parser.ParseFile (using fileSet and parser.SkipObjectResolution) without
applying build-tag constraints, which can cause duplicate-type errors when
upstream packages have platform-specific files; update the loop that calls
parser.ParseFile to first filter sourceFiles by evaluating build tags (e.g. use
go/build.Context.MatchFile or equivalent) and only parse files that match the
current build context (or add a clear comment documenting the assumption), so
that parser.ParseFile is invoked only for files valid for the active platform.
- Around line 338-347: The code uses strings.Index(name, targetPrefix) (where
targetPrefix := "/" + targetDir) which can match the suffix anywhere in the zip
entry path; instead strip the leading module@version component first and then do
an exact segment match: find the first '/' in name, set rest to the substring
after that (replace current pkgPos/rest logic), then check if rest equals
targetDir or starts with targetDir+"/" (using rest == targetDir ||
strings.HasPrefix(rest, targetDir+"/")) so you only match the top-level package
directory; update references to name, targetDir, targetPrefix, pkgPos and rest
accordingly in this block.
- Around line 129-131: Remove the redundant loop-variable re-binding inside the
test loop: in the for-range over cases, delete the unnecessary "tc := tc"
statement so each iteration uses the per-iteration scoped loop variable "tc"
directly when calling t.Run and within the subtest; update the test body that
references tc (the subtest closure passed to t.Run) to rely on the loop's
per-iteration scoping and ensure no other re-bindings remain.
ti-chi-bot
bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lidezhu, wk989898 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
What problem does this PR solve?
Issue Number: close #4125
What is changed and how it works?
This pull request enhances the project's robustness by adding a critical test that safeguards against breaking changes in upstream TiDB's schema definitions. By proactively verifying the structure of key data types, it ensures the continued compatibility and correctness of TiCDC's shared-schema functionality, thereby reducing the risk of unexpected failures during TiDB version upgrades.
Highlights
TestLatestTiDBTableInfoSharedSchemaGuard, to verify the correctness of shared schema assumptions by checking the field contracts of critical TiDB structs (TableInfo,ColumnInfo,IndexInfo,IndexColumn,FieldType) against the latest TiDB master branch.queryModuleInfo,decodeModuleInfo,extractStructFields,embeddedFieldName,diffFieldLists) to programmatically resolve module versions, parse Go source files, and compare expected struct fields with actual fields, providing detailed error messages upon mismatch.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit