-
Notifications
You must be signed in to change notification settings - Fork 128
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
Unit tests for DamageSourceMock #973
Unit tests for DamageSourceMock #973
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good that you noticed this! (I was the one that missed adding a testcase for this class).
I have some minor requested changes, but overall good.
src/test/java/be/seeseemelk/mockbukkit/damage/DamageSourceMockTest.java
Outdated
Show resolved
Hide resolved
src/test/java/be/seeseemelk/mockbukkit/damage/DamageSourceMockTest.java
Outdated
Show resolved
Hide resolved
Hi @Thorinwasher, thank you for the review! I have migrated the logic from constructor to a method DamageTypeMock#from(JsonObject) in commit 1cb49f0. I think this approach allow us to have the best of both worlds! But let me know your opinion on that :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to with the DamageTypeMock.from(JsonObject) function. Definittely better than using a constructor!
On that, I plan on making a new pull request that applies this on all other keyed objects (but that has to be in a different pull request)
src/test/java/be/seeseemelk/mockbukkit/damage/DamageTypeMockTest.java
Outdated
Show resolved
Hide resolved
src/test/java/be/seeseemelk/mockbukkit/damage/DamageTypeMockTest.java
Outdated
Show resolved
Hide resolved
src/main/java/be/seeseemelk/mockbukkit/damage/DamageTypeMock.java
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
I followed your suggestion , and I think the current state is a good trade-off between complexity and code readability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Description
Added unit tests for class DamageSourceMock (Related with #479).
This also includes commit 49e2007 which is a fix for pipeline to build.
Checklist
The following items should be checked before the pull request can be merged.