ISSUE #1039: BKClient tests with BookieErrors#1040
ISSUE #1039: BKClient tests with BookieErrors#1040reddycharan wants to merge 2 commits intoapache:masterfrom
Conversation
- end-to-end tests for validating BKClient readEntry calls incase of combination of bookie timeouts and data corruption
reddycharan
force-pushed
the
bookkeeperclienttests
branch
from
8dfb465 to
1b2b5ec
Compare
|
@reddycharan just one general comment (not related to this PR): it would be good if you could spend some time updating the description when you sent the PR (e.g. move the description down to below "Descriptions of the changes in this PR" and update the master issue). so the merge script can have a better commit message. |
sijie
left a comment
sijie
left a comment
There was a problem hiding this comment.
overall the change looks good to me. one comment that I would suggest to just test with one digest type.
| /** | ||
| * Test the bookkeeper client with errors from Bookies. | ||
| */ | ||
| public class BookKeeperClientTestsWithBookieErrors extends BaseTestCase { |
There was a problem hiding this comment.
I think it is good enough to test one time of digest type, because the test is focusing on testing bookie errors not different implementation of digest types. we unnecessarily increases the number of duplicated test cases to run, which increase the possibility of flakiness and also the testing time.
so I would suggest just test with CRC32 (or even just CRC32C), which is the commonly used.
There was a problem hiding this comment.
hmm..i thought about it. But in this testsuite, for few testcases I'm corrupting data. So I would like to make sure digest error is observed in both the digestmanagers cases. It is kind of end-to-end negative scenario for various digestmanagers.
There was a problem hiding this comment.
well. I think the point if you are testing digest manager cases, you should just test the digest manager, you don't need to involve other components. if you are testing end-to-end, you should just use one digest manager. remember all the digest manager are implementing same interface, there is no need to test different digest manager end-to-end.
I would suggest splitting the test case into two.
- you can test corrupting data on various digest managers, without involving other components.
- you can test end-to-end using one digest type.
|
@sijie will be mindful about PR description from now on. The last couple of PRs are for just testcases, so I didn't have much to mention in all of the sections. If you agree with my explanation for DigestType paramaterized test class, I guess this commit should be good to go. |
|
@reddycharan left a comment, which I would prefer if you want parameterized tests, it should be done on digest manager only. for end-to-end testing, it would be great to just test one. regarding the PR description, you can edit PR description any time after you sent the PR. |
- removed paramterized BaseTestClass
2 participants
Descriptions of the changes in this PR:
calls incase of combination of bookie timeouts and
data corruption
Master Issue: #1039