test: fix part of unit tests for next-gen#63192
Conversation
Signed-off-by: tangenta <tangenta@126.com>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
|
Hi @tangenta. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #63192 +/- ##
================================================
+ Coverage 72.7920% 74.7288% +1.9367%
================================================
Files 1831 1877 +46
Lines 495455 503697 +8242
================================================
+ Hits 360652 376407 +15755
+ Misses 112879 103882 -8997
- Partials 21924 23408 +1484
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: tangenta <tangenta@126.com>
|
|
||
| if kerneltype.IsNextGen() { | ||
| testenv.UpdateConfigForNextgen(t) | ||
| config.UpdateGlobal(func(conf *config.Config) { |
There was a problem hiding this comment.
why inline it?
we need to revert the global config back, to make sure tests depends on it work
There was a problem hiding this comment.
This function only provides the default case of mockstore.NewMockStore for next-gen. If there are some tests depending on the global config, maybe use mockstore.NewMockStore directly and call config.updateGlobal explicitly.
|
/test pull-unit-test-next-gen |
|
@wuhuizuo: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: tangenta <tangenta@126.com>
|
/test pull-unit-test-next-gen |
|
@tangenta: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: tangenta <tangenta@126.com>
ti-chi-bot
Bot
added
size/L
|
/test pull-unit-test-next-gen |
|
@wuhuizuo: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: tangenta <tangenta@126.com>
ti-chi-bot
Bot
added
size/M
Signed-off-by: tangenta <tangenta@126.com>
ti-chi-bot
Bot
added
size/L
|
/test pull-unit-test-next-gen |
|
/review default |
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Review: test: fix rest unit test for next-gen
General Observations:
- The PR introduces changes related to the "next-gen" feature flag (
kerneltype.IsNextGen()). teststore.NewMockStoreWithoutBootstrapreplaces calls tomockstore.NewMockStorein several places.- Includes new logic for handling key decoding in
closure_exec.goandmpp_exec.goconsidering API V2. - Adds conditional checks for
next-genscenarios in unit tests, skipping certain tests or altering expectations. - Minor adjustments to Bazel build files to include new dependencies.
- The PR description lacks clarity on what was changed and why, and the release note is set to "None".
Potential Problems:
1. Incomplete PR Description
- The PR description does not explain the rationale behind the changes or provide adequate context about the "next-gen" integration. This makes it harder to evaluate the scope and potential side effects.
Suggestions:
- Update the PR description to include:
- Why the
next-genchanges were necessary. - A summary of key changes and their rationale.
- Any potential backward compatibility issues or risks.
- Why the
2. Use of Conditional Checks (kerneltype.IsNextGen())
- The use of
kerneltype.IsNextGen()is pervasive and introduces branching logic throughout the codebase, which can make maintenance challenging over time. - Some of the conditional logic appears repetitive (e.g., handling key decoding).
Suggestions:
- Consider centralizing
next-genspecific logic into a utility function or module to reduce redundancy and improve readability. - Document the behavior of
kerneltype.IsNextGen()and its expected impact, especially for tests.
3. Key Decoding Logic
- The key decoding logic in
closure_exec.goandmpp_exec.gointroduces new handling for API V2 keys. While necessary fornext-gen, this logic introduces complexity and error-prone manual checks for key prefixes.
Suggestions:
- Extract key decoding logic into a reusable helper function.
- Add comprehensive unit tests for the helper function to ensure correct behavior for various key formats and edge cases.
4. Test Adjustments
- Several tests now include
t.Skip()calls when `kerneltype.IsNextGen
......
Response is trunked for length limits.
|
@wuhuizuo: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes rest unit tests for the next-gen system by adapting test expectations and infrastructure to handle differences between regular and next-gen modes. The changes primarily address differences in execution plans, key encoding, and system table configurations.
Key changes include:
- Conditional test assertions based on kernel type for different execution plans in next-gen vs regular mode
- Updated mock store creation to use teststore.NewMockStoreWithoutBootstrap for next-gen compatibility
- Improved key handling in coprocessor handlers to properly decode API V2 keys
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/stmtsummary/v2/tests/table_test.go | Added next-gen conditional checks for execution plan differences |
| pkg/testkit/mockstore.go | Updated to use teststore.NewMockStoreWithoutBootstrap for next-gen |
| pkg/store/mockstore/teststore/store.go | Refactored to remove testing dependency and handle next-gen config |
| pkg/store/mockstore/unistore/cophandler/mpp_exec.go | Improved key decoding for API V2 compatibility |
| pkg/store/mockstore/unistore/cophandler/closure_exec.go | Enhanced key handling for next-gen compatibility |
| Multiple test files | Added kerneltype imports and next-gen conditional logic |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| func (e *indexScanExec) Process(key, value []byte) error { | ||
| if kerneltype.IsNextGen() && len(key) > 4 && key[0] == 'x' { | ||
| key = key[4:] // remove the keyspace prefix | ||
| decodedKey := key | ||
| if !kv.Key(key).HasPrefix(tablecodec.TablePrefix()) { | ||
| // If the key is in API V2, then ignore the prefix | ||
| _, k, err := tikv.DecodeKey(key, kvrpcpb.APIVersion_V2) | ||
| if err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
| decodedKey = k | ||
| if !kv.Key(decodedKey).HasPrefix(tablecodec.TablePrefix()) { | ||
| return errors.Errorf("invalid index key %q after decoded", key) | ||
| } | ||
| } |
There was a problem hiding this comment.
The key decoding logic is duplicated between mpp_exec.go and closure_exec.go. Consider extracting this logic into a shared helper function to avoid code duplication and make maintenance easier.
| } | ||
|
|
||
| hexKey := p2.Key | ||
| if kerneltype.IsNextGen() { |
There was a problem hiding this comment.
The magic behavior of encoding 'nil' to get a key prefix is unclear. Consider adding a comment explaining why encoding nil produces the desired prefix, or use a more explicit method to obtain the codec prefix.
| if kerneltype.IsNextGen() { | |
| if kerneltype.IsNextGen() { | |
| // EncodeKey(nil) returns the codec's key prefix. We use this to strip the prefix from the hex key. |
|
/retest |
|
@wuhuizuo: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@D3Hunter: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: tangenta <tangenta@126.com>
…a/tidb into next-gen-fix-unit-test-2
tangenta
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, YuJuncen 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 |
ti-chi-bot
Bot
added
approved
lgtm
and removed
needs-1-more-lgtm
|
@tangenta: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@tangenta: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
|
@D3Hunter: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What problem does this PR solve?
Issue Number: ref #61702
Problem Summary:
There are many unit tests failures in next-gen build.
What changed and how does it work?
mockstore.NewMockStorewithteststore.NewMockStoreWithoutBootstrapto inject next-gen configuration code.mysqlandsysdatabase in schema checker, to fix bootstrapping "system db not found" error in the tests withmockstore.WithSchemaChecker().Cluster.SplitKeys()in tests.tidb_enable_metadata_lockas next-gen doesn't support./mvcc/hex/HTTP API in tests.indexScanProcessCoreand(e *indexScanExec) ProcessCheck List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.