qpid-proton-j icon indicating copy to clipboard operation
qpid-proton-j copied to clipboard

Replace SinkHandler by mocking object and improve test design

Open wx930910 opened this issue 3 years ago • 1 comments

Fixes PROTON-2431

Description

Refactor test class SinkHandler by mocking object and improve test design


Motivation
  • Decoupling test class SinkHandler from production class BaseHandler.
  • Making test condition more clear by verifying mocking obejct's behavior with a local variable.
  • Making test logic more clear by using method stub instead of method overriding.

Key changed/added classes in this PR
  • Created mocking object to replace test subclass SinkHandler, decoupled test from production code.
  • Extract variable received as a local variable to improve test logic and make test condition more explict.
  • Make test logic more clear by using method stub instead of method overriding.

wx930910 avatar Sep 06 '21 01:09 wx930910

I'm not sure I would actually consider this a great improvement overall. Its just as (if not more) verbose as the original but harder to understand from a simple glance, doing effectively the same thing but in a less trivially readable way. The use of BaseHandler in the original doesn't seem an issue given that is the way it was intended to be used (and remains used that way in other parts of the same test). There are other approaches the test could have originally taken to use a local variable for the count, which is what I might have done personally, but I would say the logic is trivial as-is and far from unclear even without doing so. Overall the Reactor is not a focus though either way and hasnt been for some time.

Aside, the used imports should have been added rather than a wildcard, the comments being added dont seem necessary and are more of a distraction than a help, and the alignment is clearly way off for most of the changed lines, presumably from use of tabs vs spaces in the existing file. The commit log and PR title should have referenced the JIRA key to link them. Not necessarily an issue currently as I don't plan to merge this as is.

@gemmellr Thanks for the comments! if we totally get rid of the variable received and use Mockito.verify() instead of assertion to check method execution status, will it make the test logic more explict?

wx930910 avatar Sep 09 '21 00:09 wx930910