tests/realtikvtest: tolerate next-gen lazy uniqueness conflict reasons#67506
tests/realtikvtest: tolerate next-gen lazy uniqueness conflict reasons#67506zanmato1984 wants to merge 1 commit into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
release-note-none
|
@zanmato1984 I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/S
|
Hi @zanmato1984. 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. |
📝 WalkthroughWalkthroughThis change modifies test assertions for lazy uniqueness conflict detection in pessimistic transaction tests. It replaces a hardcoded assertion with a new helper function that accepts multiple valid error reason values, enabling more flexible error validation across different execution paths. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
| strings.Contains(errMsg, "reason=LazyUniquenessCheck") || strings.Contains(errMsg, "reason=NotLockedKeyConflict"), | ||
| "expected next-gen lazy uniqueness conflict reason, got %s", |
There was a problem hiding this comment.
we should use different msg depends on kernel type to be exact
if kerneltype.IsNextGen() {...}
else ...or better, let's wait tikv side fix this
There was a problem hiding this comment.
I can't guess if tikv changes the behavior intentionally. If not, tikv should fix it, I agree.
There was a problem hiding this comment.
It's a mistake by AI in CSE PR#4440. I will fix it in tikv side.
There was a problem hiding this comment.
Thanks. I think I can close this PR now.
|
@zanmato1984: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/realtikvtest/pessimistictest/pessimistic_test.go (1)
3099-3108: Guard against nilerrbefore reading error text.Please assert
errfirst in the helper so unexpected success fails with a clear test assertion instead of a nil-pointer panic.💡 Suggested patch
func requireNextGenLazyUniquenessConflictReason(t *testing.T, err error) { t.Helper() + require.Error(t, err) errMsg := err.Error() require.Truef( t, strings.Contains(errMsg, "reason=LazyUniquenessCheck") || strings.Contains(errMsg, "reason=NotLockedKeyConflict"), "expected next-gen lazy uniqueness conflict reason, got %s", errMsg, ) }As per coding guidelines: Keep error handling actionable and contextual; avoid silently swallowing errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/realtikvtest/pessimistictest/pessimistic_test.go` around lines 3099 - 3108, The helper requireNextGenLazyUniquenessConflictReason should guard against a nil error before calling err.Error(); update the function to assert the error is non-nil (e.g., using require.Error or require.NotNil on the incoming err) and only then read err.Error() and check for the "reason=LazyUniquenessCheck" or "reason=NotLockedKeyConflict" substrings; this prevents a nil-pointer panic and yields a clear test failure when the operation unexpectedly succeeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/realtikvtest/pessimistictest/pessimistic_test.go`:
- Around line 3099-3108: The helper requireNextGenLazyUniquenessConflictReason
should guard against a nil error before calling err.Error(); update the function
to assert the error is non-nil (e.g., using require.Error or require.NotNil on
the incoming err) and only then read err.Error() and check for the
"reason=LazyUniquenessCheck" or "reason=NotLockedKeyConflict" substrings; this
prevents a nil-pointer panic and yields a clear test failure when the operation
unexpectedly succeeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f525cd60-320d-4266-9fb5-4ab50915be9f
📒 Files selected for processing (1)
tests/realtikvtest/pessimistictest/pessimistic_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67506 +/- ##
================================================
- Coverage 77.7160% 77.2054% -0.5106%
================================================
Files 1959 1942 -17
Lines 543377 543384 +7
================================================
- Hits 422291 419522 -2769
- Misses 120245 123860 +3615
+ Partials 841 2 -839
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Closing as #67506 (comment) |
3 participants
What problem does this PR solve?
Issue Number: ref #67503
Problem Summary:
pull-integration-realcluster-test-next-genstarted failing inTestLazyUniquenessCheckandTestLazyUniquenessCheckWithInconsistentReadResultafter the next-gen job switched to a different TiKV/TiKV-worker artifact family. The current TiDB tests assume every non-classic environment reportsreason=NotLockedKeyConflict, but the affected next-gen environment can reportreason=LazyUniquenessCheckfor the same lazy uniqueness conflict.What changed and how does it work?
Keep the classic assertion unchanged and relax the next-gen assertion in the two affected tests to accept either
reason=LazyUniquenessCheckorreason=NotLockedKeyConflict.This matches the current engine-family divergence without changing production behavior.
Check List
Tests
make bazel_preparego test -run '^$' -tags=intest,deadlock ./tests/realtikvtest/pessimistictest/...go test -run '^(TestLazyUniquenessCheck|TestLazyUniquenessCheckWithInconsistentReadResult)$' -tags=intest,deadlock ./tests/realtikvtest/pessimistictest/... -args -tikv-path 'tikv://127.0.0.1:12379?disableGC=true'tiup playgroundresolved tov8.5.5, whose PD API is too old for currentmasteron this machine (GetGCState/QueryRegionunimplemented), so the full RealTiKV verification needs CI.Side effects
Documentation
Release note
Summary by CodeRabbit