Skip to content
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

util/checksum: migrate to testify #26252

Merged
merged 13 commits into from
Jul 15, 2021
Merged

Conversation

Ranxy
Copy link
Contributor

@Ranxy Ranxy commented Jul 14, 2021

What problem does this PR solve?

Issue Number: close #26169

What is changed and how it works?

What's Changed:
remove pingcap/check for utils/hack
refactor duplicate code

Release note

  • No release note.

@ti-chi-bot ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 14, 2021
@Ranxy
Copy link
Contributor Author

Ranxy commented Jul 14, 2021

/run-check_dev

@Ranxy
Copy link
Contributor Author

Ranxy commented Jul 14, 2021

Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this PR @Ranxy !

May you shuffle helper functions after test functions? Always we focus on test functions first and optionally take a look at its helpers.

@ti-chi-bot
Copy link
Member

@tisonkun: Request changes is only allowed for the reviewers in list.

In response to this:

Thanks for submitting this PR @Ranxy !

May you shuffle helper functions after test functions? Always we focus on test functions first and optionally take a look at its helpers.

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.

@Ranxy
Copy link
Contributor Author

Ranxy commented Jul 14, 2021

@tisonkun Updated.

@tisonkun tisonkun self-requested a review July 14, 2021 15:38
Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

util/checksum/checksum_test.go Outdated Show resolved Hide resolved
@tisonkun
Copy link
Contributor

@Ranxy I'm creating a PR for you, please wait to make other changes.

@tisonkun
Copy link
Contributor

@Ranxy you can take a look at Ranxy#1 .

Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created a PR for you. You can take a look at Ranxy#1 .

Basically it reduces unnecessary file operation and improve ticase comments.

@Ranxy
Copy link
Contributor Author

Ranxy commented Jul 15, 2021

@tisonkun Your modifications are very effective, and I think @xhebox's proposal is also very good, and we can completely replace File with the following bytes.buffer. which do you think is better?

type fakeFile struct {
	buf *bytes.Buffer
}
func (f *fakeFile) Write(p []byte) (n int, err error) {
	return f.buf.Write(p)
}
func (f *fakeFile) Close() error {
	return nil
}
func (f *fakeFile) ReadAt(p []byte, off int64) (n int, err error) {
	w := f.buf.Bytes()
	lp := int64(len(p))
	lw := int64(len(w))
	if off > lw {
		return 0, io.EOF
	}
	if off+lp > lw {
		copy(p[:lw-off], w[off:])
		return int(lw - off), io.EOF
	}
	copy(p, w[off:off+lp])
	return len(p), nil
}

@tisonkun
Copy link
Contributor

tisonkun commented Jul 15, 2021

@Ranxy you can integrate two comments and I think Close for byte buffer is redundant.

The parallelism refactor and ticase comment update in the PR Ranxy#1 are still valid and should be included in the final change set IMO :P

@Ranxy
Copy link
Contributor Author

Ranxy commented Jul 15, 2021

/run-check_dev_2

Copy link
Contributor

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot
Copy link
Member

@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.

Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

util/checksum/checksum_test.go Outdated Show resolved Hide resolved
simplification check file is same
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 15, 2021
@Ranxy
Copy link
Contributor Author

Ranxy commented Jul 15, 2021

/run-check_dev_2

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • tiancaiamao
  • xhebox

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 ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 15, 2021
@Ranxy
Copy link
Contributor Author

Ranxy commented Jul 15, 2021

/run-check_dev_2

@Ranxy
Copy link
Contributor Author

Ranxy commented Jul 15, 2021

@tiancaiamao @xhebox Thanks for your review! May you help with merging this PR?

@tiancaiamao
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: ddd56a0

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 15, 2021
@ti-chi-bot ti-chi-bot merged commit fd06dbf into pingcap:master Jul 15, 2021
@Ranxy Ranxy deleted the checksum-testify branch July 15, 2021 13:04
} else {
c.Assert(err, check.Equals, errChecksumFail)
assert.ErrorIs(t, err, errChecksumFail)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line now creates unstable test

[2021-09-23T16:00:22.934Z] --- FAIL: TestModifyOneByte (0.00s)
[2021-09-23T16:00:22.934Z]     --- FAIL: TestModifyOneByte/encrypted (0.00s)
[2021-09-23T16:00:22.934Z]         checksum_test.go:179: 
[2021-09-23T16:00:22.934Z]             	Error Trace:	checksum_test.go:179
[2021-09-23T16:00:22.934Z]             	            				checksum_test.go:150
[2021-09-23T16:00:22.934Z]             	Error:      	Target error should be in err chain:
[2021-09-23T16:00:22.934Z]             	            	expected: "error checksum"
[2021-09-23T16:00:22.934Z]             	            	in chain: 
[2021-09-23T16:00:22.934Z]             	Test:       	TestModifyOneByte/encrypted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

migrate test-infra to testify for util/checksum pkg
5 participants