VStreamer unit tests: refactor pending test #15845
Merged
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
rohit-nayak-ps
removed
NeedsDescriptionUpdate
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15845 +/- ##
=======================================
Coverage 68.45% 68.45%
=======================================
Files 1559 1559
Lines 196825 196825
=======================================
+ Hits 134736 134737 +1
+ Misses 62089 62088 -1 ☔ View full report in Codecov by Sentry. |
rohit-nayak-ps
requested review from
harshit-gangal,
systay and
shlomi-noach
as code owners
frouioui
approved these changes
May 6, 2024
mattlord
approved these changes
May 7, 2024
| @@ -511,8 +511,12 @@ func (ts *TestSpec) getFieldEvent(table *schemadiff.CreateTableEntity) *TestFiel | |||
| tc.len = int64(len(col.Type.EnumValues) + 1) | |||
| tc.colType = fmt.Sprintf("%s(%s)", tc.dataTypeLowered, strings.Join(col.Type.EnumValues, ",")) | |||
| ts.metadata[getMetadataKey(table.Name(), tc.name)] = col.Type.EnumValues | |||
| case "json": | |||
| tc.colType = "json" | |||
| tc.len = lengthJSON | |||
There was a problem hiding this comment.
You probably already know, but this is the length of a longblob (2^32 - 1) type because that's what is used within InnoDB for the JSON storage:
- https://dev.mysql.com/doc/refman/8.0/en/storage-requirements.html#data-types-storage-reqs-json
- https://dev.mysql.com/doc/refman/8.0/en/storage-requirements.html#data-types-storage-reqs-strings
Might be worth a comment since some of the length consts we have are not for a given type but the current usage (e.g. the SET ones).
It's only relevant in that these lengths are tied to types. So at some point we may add mediumblob/mediumtext longblob/longtext etc fields too. So I'm only thinking of how this might be (more) obvious to future readers / writers.
| default: | ||
| log.Infof(fmt.Sprintf("unknown sqlTypeString %s", tc.dataTypeLowered)) | ||
| ts.t.Fatalf("unknown sqlTypeString %s", tc.dataTypeLowered) |
There was a problem hiding this comment.
We should not be using t.Fatal in new code but using require.Fail[Now] instead. We've had efforts to remove/replace this usage across the code base in the past so we shouldn't introduce new usage -- that's my thinking.
| for l < col.len { | ||
| val = append(val, "\x00"...) | ||
| l++ | ||
| } | ||
| case "json": | ||
| sval := strings.Trim(string(val), "'") | ||
| sval = strings.Replace(sval, "\\", "", -1) |
There was a problem hiding this comment.
Nit, but IMO strings.ReplaceAll is a bit more clear.
systay
changed the title
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
… for now Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
rohit-nayak-ps
force-pushed
the
rohit/vstreamer-unit-test-refactor4
branch
from
6749832
to
778ea66
Compare
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
Signed-off-by: Rohit Nayak <rohit@planetscale.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
Final PR on porting over the vstreamer unit tests. Only one test
TestJSONhas been ported. Remaining ones are too specific and will require disproportional changes in the test framework for each test and those have been left as-is.Related Issue(s)
#14903
Checklist
Deployment Notes