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

add unit tests for WriteBatch #144

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Conversation

pfi79
Copy link
Contributor

@pfi79 pfi79 commented Jan 18, 2025

@pfi79 pfi79 requested a review from a team as a code owner January 18, 2025 16:54
@pfi79
Copy link
Contributor Author

pfi79 commented Jan 18, 2025

@bestbeforetoday Take a look. What other unit tests are missing.

@bestbeforetoday
Copy link
Member

bestbeforetoday commented Jan 20, 2025

Generally coverage looks good. A huge improvement on what was there before. The only case I noticed missing is that starting a write batch twice doesn't lose any previously batched writes.

An area that could (optionally) be improved is how specific the tests are. Right now, each unit tests tends to perform a significant number of operations. This means that failures are quite generic and don't pinpoint specific problems in the code well, meaning that debugging of the code is probably going to be needed to identify the actual problem and fix any test failure. The smaller and more targeted each unit test is, the more accurately the detected issue can be identified and more quickly it can be fixed. The ideal would be that a test failure message would be enough information to know exactly the conditions that failed and therefore be able to identify the faulty code without any debugging. Perhaps that's just something to consider for future tests though.

@pfi79
Copy link
Contributor Author

pfi79 commented Jan 20, 2025

The only case I noticed missing is that starting a write batch twice doesn't lose any previously batched writes.

added

Signed-off-by: Fedor Partanskiy <[email protected]>
@pfi79
Copy link
Contributor Author

pfi79 commented Jan 20, 2025

@denyeart look at this pr. I think we can put a tag after it. And we can move on to fabric changes.

@denyeart denyeart merged commit 02a7d5b into hyperledger:main Jan 21, 2025
6 checks passed
@pfi79 pfi79 deleted the add-unit-test branch January 21, 2025 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants