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

store/mockstore: Add raw delete range to mock store #6156

Closed

Conversation

MyonKeminta
Copy link
Contributor

It's required to test raw delete range API, which will be added to ticlient

zhangjinpeng87
zhangjinpeng87 previously approved these changes Mar 28, 2018
Copy link
Contributor

@zhangjinpeng87 zhangjinpeng87 left a comment

Choose a reason for hiding this comment

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

LGTM

@zhangjinpeng87
Copy link
Contributor

@coocood PTAL

@coocood
Copy link
Member

coocood commented Mar 28, 2018

@MyonKeminta
Please add tests for this feature.

@MyonKeminta
Copy link
Contributor Author

MyonKeminta commented Mar 28, 2018

@coocood Where to put the test?
It seems the other methods here don't have any tests yet either.

@coocood
Copy link
Member

coocood commented Mar 28, 2018

@MyonKeminta
See mock_tikv_test.go

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 30, 2018
@MyonKeminta
Copy link
Contributor Author

@tiancaiamao @coocood PTAL

@coocood
Copy link
Member

coocood commented Apr 3, 2018

go fmt ./...

// testMVCCLevelDB is used to test MVCCLevelDB implementation.
type testMVCCLevelDB struct {
testMockTiKVSuite
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Friendly remind: avoid to move code if it's not necessary.
Because it would make the code reviewer's work easier.

@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao
Copy link
Contributor

/run-all-tests

Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

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

LGTM

@tiancaiamao
Copy link
Contributor

/run-integration-common-test

@MyonKeminta
Copy link
Contributor Author

.....It seems that changes in this pr was merged with #6157 ..
This pr cam be closed

@coocood coocood closed this Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants