Improve WaitForPos errors, don't include Result struct in message #15962
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
vitess-bot
bot
added
NeedsBackportReason
rafer
force-pushed
the
wait-for-pos-error-test-refactor
branch
2 times, most recently
from
ee2b51d
to
c3d4c57
Compare
Signed-off-by: Rafer Hazen <rafer@ralua.com>
rafer
force-pushed
the
wait-for-pos-error-test-refactor
branch
from
c3d4c57
to
6053c5e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15962 +/- ##
==========================================
+ Coverage 68.47% 68.61% +0.13%
==========================================
Files 1562 1562
Lines 197062 198642 +1580
==========================================
+ Hits 134933 136289 +1356
- Misses 62129 62353 +224 ☔ View full report in Codecov by Sentry. |
dbussink
reviewed
May 17, 2024
| @@ -426,7 +426,8 @@ func TestWaitForPosError(t *testing.T) { | |||
|
|
|||
| dbClient.ExpectRequest("select pos, state, message from _vt.vreplication where id=1", &sqltypes.Result{Rows: [][]sqltypes.Value{{}}}, nil) | |||
| err = vre.WaitForPos(context.Background(), 1, "MariaDB/0-1-1084") | |||
| want = "unexpected result: &{[] 0 0 [[]] 0 }" | |||
| want = fmt.Sprintf("unexpected result: %v", &sqltypes.Result{Rows: [][]sqltypes.Value{{}}}) | |||
There was a problem hiding this comment.
@rafer I think we should actually fix the error message generated here. I don't think it's very clear what this is in the first place. The error message does not seem very user friendly if it contains &{[] 0 0 [[]] 0 }. I don't think that's meaningful in any way for a Vitess user?
Something along these lines maybe, so instead of:
case len(qr.Rows) > 1 || len(qr.Rows[0]) != 3:
return fmt.Errorf("unexpected result: %v", qr)We do something like:
case len(qr.Rows) > 1:
return fmt.Errorf("more rows received than expected, got %v instead of 1", len(qr.Rows))
case len(qr.Rows[0]) != 3:
return fmt.Errorf("more columns received than expected, got %v instead of 3", len(qr.Rows[0]))There was a problem hiding this comment.
Alternatively, just printing qr.Rows instead of the whole qr already makes it look a lot better as well, since rows have a proper .String() implementation for debugging.
Signed-off-by: Rafer Hazen <rafer@ralua.com>
rafer
changed the title
mattlord
added
Type: Internal Cleanup
Type: Enhancement
mattlord
approved these changes
May 21, 2024
Comment on lines
794
to
796
| return fmt.Errorf("vreplication stream received more rows than expected, got %v instead of 1", len(qr.Rows)) | ||
| case len(qr.Rows[0]) != 3: | ||
| return fmt.Errorf("vreplication stream received an unexpected number of columns, got %v instead of 3", len(qr.Rows[0])) |
There was a problem hiding this comment.
Super nitty, but we could use %d instead of %v here.
dbussink
approved these changes
May 21, 2024
Signed-off-by: Rafer Hazen <rafer@ralua.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add more specific errors when
WaitForPosreceives an unexpected number of rows or columns. No longer prints the raw Result object (which isn't a particularly descriptive error for end users).Immediate motivation is to make tests less brittle when
Resultobject is modified (in downstream forks).Related Issue(s)
N/A
Checklist
Deployment Notes