Skip to content

CODEC-303: Refactor NoOpBaseNCodec to improve test logic#94

Closed
wx930910 wants to merge 1 commit intoapache:masterfrom
wx930910:CODEC-303
Closed

CODEC-303: Refactor NoOpBaseNCodec to improve test logic#94
wx930910 wants to merge 1 commit intoapache:masterfrom
wx930910:CODEC-303

Conversation

@wx930910
Copy link

Jira

Description

Replace test class NoOpBaseNCodec by mocking object and improve test design


Motivation
  • Decouple test class NoOpBaseNCodec from production class BaseNCodec.
  • Remove the redundant overridden methods.
  • Make testing logic more explict.

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.

@wx930910
Copy link
Author

We can also get rid of the method that return the mocking object and directly create the mocking object inside of each test case.

@garydgregory
Copy link
Member

garydgregory commented Sep 22, 2021

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.

@wx930910
Copy link
Author

@garydgregory, thanks for providing the feedback! What do you think about this case, 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.

@garydgregory
Copy link
Member

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.

@wx930910
Copy link
Author

wx930910 commented Sep 27, 2021

@garydgregory I see, thank you so much for providing such detailed feedback!

@garydgregory
Copy link
Member

Closing per comments.

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.

2 participants