hap-java / HAP-Java

Java implementation of the HomeKit Accessory Protocol
MIT License
155 stars 82 forks source link

Removed 2 unnecessary stubbings in HomekitRootTest.java #187

Closed ARUS2023 closed 11 months ago

ARUS2023 commented 1 year ago

Pull Request Checklist

Please confirm that you've done the following when opening a new pull request:

In our analysis of the project, we observed that

1) 1 stubbing that stubbed getId is created in HomekitRootTest.setup but never executed in 5 tests HomekitRootTest.testWebHandlerStarts, HomekitRootTest.testWebHandlerStops, HomekitRootTest.testAdvertiserStarts, HomekitRootTest.testAdvertiserStops, HomekitRootTest.testAddIndexOneAccessory; 2) 1 stubbing that stubbed startis created in HomekitRootTest.setup but never executed in 3 testsHomekitRootTest.verifyRegistryAdded,HomekitRootTest.verifyRegistryRemoved, HomekitRootTest.testAddIndexOneAccessory.

Unnecessary stubbings are stubbed method calls that were never realized during test execution. Mockito recommends to remove unnecessary stubbings (https://www.javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/exceptions/misusing/UnnecessaryStubbingException.html).

We propose below a solution to remove the unnecessary stubbing.

ccutrer commented 1 year ago

Why did you split HomeKitRootTest.java into 4 files. If they're split by functionality, please name them more descriptively than "second", "third", "fourth".

ARUS2023 commented 1 year ago

Hello,

We divided the HomeKitRootTest.java into four separate files to enhance the test organization and eliminate unnecessary stubbings in the setUp method. This separation allows us to ensure that stubbing is only created when the tests actually use it.

To clarify, the reasons for splitting these files are as follows:

  1. SecondHomeKitRootTest.java: The tests in this file do not execute the stubbing that is used to stub the getId method in the setUp method.
  2. ThirdHomeKitRootTest.java: Similarly, the tests in this file do not execute both stubbings in the setUp method.
  3. FourthHomeKitRootTest.java: In this case, the tests do not execute the stubbing that stubs the start method in the setUp method.

We understand that the class names may not be as descriptive as desired. We created and used an automated tool to identify and resolve unnecessary stubbings. If you would like us to provide more meaningful names for these test classes, please let us know, and we will be happy to update them accordingly.

ccutrer commented 1 year ago

Okay, so you're saying the stubbings are not used in some specs, not that they're not used at all. I'd much rather have a few sometimes-unnecessary stubbings than introduce all of the duplication of near-identical setups, and no clear reason why some tests are in one file and not another. If you can address those issues, I'll consider this PR.

ARUS2023 commented 1 year ago

Thank you for your reply. We understand your preference. As we mentioned before, we have created and used an automated tool. This tool identifies and removes unnecessary stubbings, but importantly, it also empowers developers with a choice: they can either duplicate the test class to address the issue or retain the original structure.

We believe this solution is suitable because we group the tests into different test classes according to the settings/prerequisites required by the tests. We submitted this pull request because (in the past and in a different project) we found a case in which deleting an unnecessary stub led to a problem in the test.