You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/09/22 03:48:05 UTC
[GitHub] [commons-codec] wx930910 opened a new pull request #94: CODEC-303: Refactor NoOpBaseNCodec to improve test logic
wx930910 opened a new pull request #94:
URL: https://github.com/apache/commons-codec/pull/94
### Jira
- [CODEC-303](https://issues.apache.org/jira/browse/CODEC-303)
### Description
#### Replace test class [NoOpBaseNCodec](https://github.com/apache/commons-codec/blob/b70e177f8733e9dd4e5f503620ed0b136b74785b/src/test/java/org/apache/commons/codec/binary/BaseNCodecTest.java#L375) by mocking object and improve test design
<hr>
##### Motivation
* Decouple test class `NoOpBaseNCodec` from production class `BaseNCodec`.
* Remove the redundant overridden methods.
* Make testing logic more explict.
<hr>
##### Key changed/added classes in this PR
* Created mocking object to replace test subclass `NoOpBaseNCodec`, decoupled test from production code.
* Remove test subclass `NoOpBaseNCodec`.
* Created method that return the mocking object for code reuse.
* Add Mockito dependency.
<hr>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-codec] garydgregory commented on pull request #94: CODEC-303: Refactor NoOpBaseNCodec to improve test logic
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #94:
URL: https://github.com/apache/commons-codec/pull/94#issuecomment-924597345
While this seems like a clever use of Mockito, it hinders ease of understanding the code and maintenance IMO.
Mocking is a great technique for certain use cases, this isn't one of them IMO. The current noop adapter is clear, simple, and can be stepped into while debugging if needes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-codec] garydgregory edited a comment on pull request #94: CODEC-303: Refactor NoOpBaseNCodec to improve test logic
Posted by GitBox <gi...@apache.org>.
garydgregory edited a comment on pull request #94:
URL: https://github.com/apache/commons-codec/pull/94#issuecomment-924597345
While this seems like a clever use of Mockito, it hinders ease of understanding the code and maintenance IMO.
Mocking is a great technique for certain use cases, this isn't one of them IMO. The current noop adapter is clear, simple, and can be stepped into while debugging if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-codec] garydgregory closed pull request #94: CODEC-303: Refactor NoOpBaseNCodec to improve test logic
Posted by GitBox <gi...@apache.org>.
garydgregory closed pull request #94:
URL: https://github.com/apache/commons-codec/pull/94
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-codec] wx930910 edited a comment on pull request #94: CODEC-303: Refactor NoOpBaseNCodec to improve test logic
Posted by GitBox <gi...@apache.org>.
wx930910 edited a comment on pull request #94:
URL: https://github.com/apache/commons-codec/pull/94#issuecomment-927403254
@garydgregory I see, thank you so much for providing such detailed feedback!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-codec] wx930910 commented on pull request #94: CODEC-303: Refactor NoOpBaseNCodec to improve test logic
Posted by GitBox <gi...@apache.org>.
wx930910 commented on pull request #94:
URL: https://github.com/apache/commons-codec/pull/94#issuecomment-927403254
@garydgregory I see, thanks you much for providing such detailed feedback!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-codec] garydgregory commented on pull request #94: CODEC-303: Refactor NoOpBaseNCodec to improve test logic
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #94:
URL: https://github.com/apache/commons-codec/pull/94#issuecomment-925450260
Using Mockito here I feel falls under the old adage "Once you have a hammer, everything starts to look like a nail." The class in question is an abstract class and therefore cannot be instantiated, the proper use case is subclassing, both for an app and therefore for a test. Using Mockito to avoid using a concrete subclass only obfuscates what the test is trying to do, makes the code harder to maintain and to learn as you are now requiring new users and maintainers to learn Mockito on top of learning the code base.
In other code bases, a noop concrete type might be part of the main tree instead of in the test tree and this whole thread would likely not even come up.
IOW, the shortcut Mockito affords is not worth the complexity it requires to learn and maintain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-codec] wx930910 commented on pull request #94: CODEC-303: Refactor NoOpBaseNCodec to improve test logic
Posted by GitBox <gi...@apache.org>.
wx930910 commented on pull request #94:
URL: https://github.com/apache/commons-codec/pull/94#issuecomment-924600757
@garydgregory, thanks for providing the feedback! What do you think about [this case](https://github.com/apache/commons-jci/blob/4006b2c36ccd5aac05f844f4b86895370afa36af/fam/src/test/java/org/apache/commons/jci2/monitor/FilesystemAlterationMonitorTestCase.java#L128), do you think we will get benefit from Mockito by replacing the inheritance with spying object? IMO It seems like the only reason we use inheritance is because we cannot directly create an instance from abstract class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-codec] wx930910 commented on pull request #94: CODEC-303: Refactor NoOpBaseNCodec to improve test logic
Posted by GitBox <gi...@apache.org>.
wx930910 commented on pull request #94:
URL: https://github.com/apache/commons-codec/pull/94#issuecomment-924558584
We can also get rid of the method that return the mocking object and directly create the mocking object inside of each test case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-codec] garydgregory commented on pull request #94: CODEC-303: Refactor NoOpBaseNCodec to improve test logic
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #94:
URL: https://github.com/apache/commons-codec/pull/94#issuecomment-945134566
Closing per comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org