pkg/util/workloadrepo: stabilize flaky TestCreatePartition#67793
pkg/util/workloadrepo: stabilize flaky TestCreatePartition#67793flaky-claw wants to merge 2 commits intopingcap:masterfrom
Conversation
ti-chi-bot
Bot
added
release-note-none
|
@flaky-claw 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/M
|
Hi @flaky-claw. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded BUILD test dependencies for session API and refactored workloadrepo tests to run DDL via ChangesBuild configuration
Workload repo tests (partitioning)
Sequence Diagram(s)(none) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)Command failed 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67793 +/- ##
================================================
- Coverage 77.6079% 76.8804% -0.7275%
================================================
Files 1982 1975 -7
Lines 548895 564592 +15697
================================================
+ Hits 425986 434061 +8075
- Misses 122099 129465 +7366
- Partials 810 1066 +256
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/test mysql-test |
|
@yinsustart: PRs from untrusted users cannot be marked as trusted with 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. |
| func anchorPartitionTestTime(now time.Time) time.Time { | ||
| return time.Date(now.Year(), now.Month(), now.Day(), 12, 0, 0, 0, now.Location()) | ||
| } |
There was a problem hiding this comment.
I don't understand why is this necessary?
At the very least, this function is only called from one place, so it doesn't need to be a new function: just replace the function call with this code instead (and add a comment if it's still not clear what it's doing).
There was a problem hiding this comment.
Ah, okay I think I see now. This seems like it is ensuring that the now timestamp we use for testing is always 12:00, so that adding/subtracting days is consistent.
Please add a comment that explains this is due to the unstable wall-clock day boundaries. The name anchorPartitionTestTime by itself doesn't clearly indicate this.
Alternatively, consider using time.Round instead of doing this rounding manually.
| func createTableWithPartsForSession(ctx context.Context, t *testing.T, se sessionapi.Session, tbl *repositoryTable, | ||
| sess sessionctx.Context, partitions []time.Time) { | ||
| createSQL, err := buildCreateQuery(ctx, sess, tbl) | ||
| require.NoError(t, err) | ||
| createSQL += buildPartitionString(partitions) | ||
| session.MustExec(t, se, createSQL) | ||
| require.True(t, validatePartitionsMatchExpected(ctx, t, sess, tbl, partitions)) | ||
| } |
There was a problem hiding this comment.
This is unnecessarily copy/pasting duplicate code from createTableWithParts.
Either modify createTableWithParts to accept an optional session, or refactor the common parts of these functions into a helper function, and have both createTableWithParts and createTableWithPartsForSession call that helper function.
There was a problem hiding this comment.
|
/check-issue-triage-complete |
|
/retest |
|
@wuhuizuo: PRs from untrusted users cannot be marked as trusted with 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. |
4 participants
What problem does this PR solve?
Issue Number: close #67461
Problem Summary:
Flaky test
TestCreatePartitioninpkg/util/workloadrepointermittently fails, so this PR stabilizes that path.What changed and how does it work?
Root Cause
TEST_ISSUE: the original flake is
TestCreatePartitionusing unstable wall-clock day boundaries, and this address round fixes only the missing Bazel strict deps introduced by the prior plain-session test fix.Fix
Adding
//pkg/sessionand//pkg/session/sessionapitoworkloadrepo_testis necessary so the existingworker_test.goimports build under TiDB's Bazel strict-deps validation.Verification
Spec:
pkg/util/workloadrepo :: TestCreatePartitiontidb.go_flaky.defaultBASELINE_ONLYObserved result:
Required flaky gate passed.
Build safety gate passed.
Intent guard gate passed.
Gate checklist:
Commands:
go test -json ./pkg/util/workloadrepo -run '^TestCreatePartition$' -count=1go test -json ./pkg/util/workloadrepo -count=1make buildCheck List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Fixes #67461
Summary by CodeRabbit
Tests
Chores