util/rowcodec: migrate test-infra to testify #27048

UBarney · 2021-08-10T03:14:25Z

What problem does this PR solve?

Issue Number: close #26413

Release note

None

ti-chi-bot · 2021-08-10T03:14:26Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2021-08-10T03:14:33Z

Welcome @UBarney!

It looks like this is your first PR to pingcap/tidb 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to pingcap/tidb. 😃

ti-chi-bot · 2021-08-10T07:04:39Z

@UBarney: GitHub didn't allow me to request PR reviews from the following users: reviewer.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @Reviewer

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/test-infra repository.

UBarney · 2021-08-10T07:06:39Z

/cc @tisonkun

tisonkun

Generally looks good. Suggestions on tests organizing.

util/rowcodec/rowcodec_test.go

Signed-off-by: tison <wander4096@gmail.com>

tisonkun

LGTM

ti-chi-bot · 2021-08-11T10:43:21Z

@tisonkun: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM

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 ti-community-infra/tichi repository.

tisonkun · 2021-08-11T10:44:19Z

/cc @xhebox @tiancaiamao @mmyj

PTAL.

It seems CI fails due to jenkins failure.

ti-chi-bot · 2021-08-11T10:44:21Z

@tisonkun: GitHub didn't allow me to request PR reviews from the following users: mmyj.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @xhebox @tiancaiamao @mmyj

PTAL.

It seems CI fails due to jenkins failure.

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/test-infra repository.

xhebox · 2021-08-12T05:11:40Z

util/rowcodec/rowcodec_test.go

-	dtNilData := []testData{
+}
+
+func TestNilAndDefault(t *testing.T) {


Maybe add this test as a subtest into TestTypesNewRowCodec. The diff of this PR is hard to review. Inline expands the code too much.

In such case, I would like to see reusing as much as possible, e.g. merging two tests of encodeAndDecode into one.

…wCodec

UBarney

hi @tisonkun, I have some questions. Please answer when you have time

UBarney · 2021-08-16T11:06:49Z

util/rowcodec/rowcodec_test.go

-var _ = Suite(&testSuite{})
-
-type testSuite struct{}
-
 type testData struct {


what's diffierence between dt and bt

Does testData.def mean default value ?

I'd like to refer to the original author @lysu . Any idea on this snippet of code?

dt should be "input" and bt should be "output"..sorry for use such short name 😭

for example, some type like duration input will decode as flatten type -- int in this rowcodec level(but will unflatten later in little upper level

tidb/util/rowcodec/rowcodec_test.go

Lines 426 to 427 in da5574a

types.NewDurationDatum(getDuration("4:00:00")),

types.NewIntDatum(14400000000000),

yes def means default value and only this case use it

tidb/util/rowcodec/rowcodec_test.go

Line 656 in da5574a

getDatumPoint(types.NewUintDatum(9)),

Got it; Thanks

@UBarney we can rename the variable to fix the readability in this pass, though - by pushing a dedicated commit.

Ok, I'll do it

how about we change testData to

type testData struct { id int64 ft *types.FieldType inputDatum types.Datum expected types.Datum defaultDatum *types.Datum handle bool }

I'd prefer

type testData struct { id int64 ft *types.FieldType input types.Datum output types.Datum default *types.Datum handle bool }

we don't have to repeat the field type and keep input and output symmetric. An output from testdata is expected data obviously IMO.

tisonkun · 2021-08-18T12:06:25Z

Please fix the wrong license header, you may try to use license header from other files that pass the check.

tiancaiamao · 2021-08-23T13:55:35Z

If it's ready for review, please remove the Draft @UBarney

UBarney · 2021-08-24T00:30:56Z

Sorry, I have been busy recently. There is no time to change this pr. Close it for now

tisonkun · 2021-08-24T04:04:51Z

@UBarney ok. I'll take it over.

util/rowcodec: migrate test-infra to testify

6dfb1bb

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 10, 2021

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 10, 2021

UBarney marked this pull request as ready for review August 10, 2021 03:23

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2021

ti-chi-bot requested a review from tisonkun August 10, 2021 07:06

tisonkun reviewed Aug 10, 2021

View reviewed changes

util/rowcodec/rowcodec_test.go Show resolved Hide resolved

util/rowcodec/rowcodec_test.go Outdated Show resolved Hide resolved

util/rowcodec/rowcodec_test.go Outdated Show resolved Hide resolved

util/rowcodec/rowcodec_test.go Outdated Show resolved Hide resolved

UBarney force-pushed the codecMigTest branch from ae323d6 to f411059 Compare August 11, 2021 07:36

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 11, 2021

util/rowcodec: refactor rowcodec_test.go

ad373b9

UBarney force-pushed the codecMigTest branch from f411059 to ad373b9 Compare August 11, 2021 07:45

UBarney requested a review from tisonkun August 11, 2021 07:49

naming

6dc0cfc

Signed-off-by: tison <wander4096@gmail.com>

tisonkun approved these changes Aug 11, 2021

View reviewed changes

ti-chi-bot requested a review from tiancaiamao August 11, 2021 10:44

ti-chi-bot requested a review from xhebox August 11, 2021 10:44

tisonkun added 2 commits August 11, 2021 18:44

Merge branch 'master' into codecMigTest

2517a04

Merge branch 'master' into codecMigTest

7ca89c9

xhebox reviewed Aug 12, 2021

View reviewed changes

UBarney marked this pull request as draft August 15, 2021 13:59

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 15, 2021

rowcodec_test: add TestNilAndDefault as a subtest into TestTypesNewRo…

11703e4

…wCodec

UBarney force-pushed the codecMigTest branch from 530340e to 11703e4 Compare August 16, 2021 11:03

UBarney commented Aug 16, 2021

View reviewed changes

Merge branch 'master' into codecMigTest

635a295

UBarney closed this Aug 24, 2021

tisonkun mentioned this pull request Aug 24, 2021

util/rowcodec: migrate test-infra to testify #27525

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util/rowcodec: migrate test-infra to testify #27048

util/rowcodec: migrate test-infra to testify #27048

UBarney commented Aug 10, 2021

ti-chi-bot commented Aug 10, 2021

ti-chi-bot commented Aug 10, 2021

ti-chi-bot commented Aug 10, 2021

UBarney commented Aug 10, 2021

tisonkun left a comment

tisonkun left a comment

ti-chi-bot commented Aug 11, 2021

tisonkun commented Aug 11, 2021

ti-chi-bot commented Aug 11, 2021

xhebox Aug 12, 2021

UBarney left a comment •

edited

Loading

UBarney Aug 16, 2021

tisonkun Aug 16, 2021

lysu Aug 16, 2021 •

edited

Loading

UBarney Aug 17, 2021

tisonkun Aug 17, 2021

UBarney Aug 17, 2021

UBarney Aug 18, 2021

tisonkun Aug 18, 2021

tisonkun commented Aug 18, 2021

tiancaiamao commented Aug 23, 2021

UBarney commented Aug 24, 2021

tisonkun commented Aug 24, 2021

	types.NewDurationDatum(getDuration("4:00:00")),
	types.NewIntDatum(14400000000000),

util/rowcodec: migrate test-infra to testify #27048

util/rowcodec: migrate test-infra to testify #27048

Conversation

UBarney commented Aug 10, 2021

What problem does this PR solve?

Release note

ti-chi-bot commented Aug 10, 2021

ti-chi-bot commented Aug 10, 2021

ti-chi-bot commented Aug 10, 2021

UBarney commented Aug 10, 2021

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Aug 11, 2021

tisonkun commented Aug 11, 2021

ti-chi-bot commented Aug 11, 2021

Choose a reason for hiding this comment

UBarney left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lysu Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tisonkun commented Aug 18, 2021

tiancaiamao commented Aug 23, 2021

UBarney commented Aug 24, 2021

tisonkun commented Aug 24, 2021

UBarney left a comment •

edited

Loading

lysu Aug 16, 2021 •

edited

Loading