-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
VStreamer Unit Tests: framework to remove the need to specify serialized strings in row events for unit tests #14903
VStreamer Unit Tests: framework to remove the need to specify serialized strings in row events for unit tests #14903
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
40d4575
to
fe2a2c8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14903 +/- ##
==========================================
- Coverage 67.47% 67.47% -0.01%
==========================================
Files 1561 1561
Lines 193186 193185 -1
==========================================
- Hits 130353 130351 -2
- Misses 62833 62834 +1 ☔ View full report in Codecov by Sentry. |
6da7aae
to
d6d95ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! Just some concerns about modifying unrelated packages' logging behavior — I think that we should remove those changes.
Otherwise I only had suggestions on potential minor improvements. Let me know what you think.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is awesome, made a few minor comments and pending @mattlord's review comments i'm good to go.
thanks @rohit-nayak-ps !!
…e moment we can infer the required test events from the sql query. If this changes we will require tests to optionally pass more info Signed-off-by: Rohit Nayak <rohit@planetscale.com>
…re custom logic that cannot be determined just from the query Signed-off-by: Rohit Nayak <rohit@planetscale.com>
…e statement Signed-off-by: Rohit Nayak <rohit@planetscale.com>
… Refactor Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
935adf5
to
646974e
Compare
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I think it's worth adding the noted comment about collationID 45, but also not necessary if there's nothing else you want to change (to avoid the CI overhead -- if so I could add that comment in another open PR after merging in main again).
Thanks!
func getDefaultCollationID() int64 { | ||
return 45 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a comment that 45 is utf8mb4_general_ci
just to avoid others having to look it up in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here: 981dc0b
…e the need to specify serialized strings in row events for unit tests (vitess#14903) (#1693) * synchronize cobradocs with vitessio/vitess#14903 * revert unwanted change Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> --------- Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr>
Description
This PR introduces a new framework to define the vstreamer unit test events in a structured manner, eliminating the need for specifying serialized row event strings directly.
This makes our tests less brittle by automating event generation for straightforward queries, thus reducing the need for extensive manual adjustments whenever row events change. While not all tests will benefit from this method, it's a significant step towards simplifying our testing process.
We plan to further refine this framework in future PRs, aiming for a balance to avoid unnecessary complexity. The framework is designed for generic applicability across multiple tests wherever feasible.
The tests currently modified are: TestNoBlob, TestSetAndEnum, TestFilteredVarBinary, TestFilteredInt, TestCellValuePadding, TestSetStatement, TestSetForeignKeyCheck, TestSavepoint, TestSavepointWithFilter.
Related Issue(s)
Checklist