only create sqlSegmentsMetadataManager once to speed up test

[TestBoost]

Description

This pull request tries to only create sqlSegmentsMetadataManager once to speed up the tests in the test class SqlSegmentsMetadataManagerEmptyTest.

There are two tests in the test class SqlSegmentsMetadataManagerEmptyTest. The first test testPollEmpty tries to assert the contents of the segments in the sqlSegmentsMetadataManager are empty. The second test testStopAndStart tries to start and stop the sqlSegmentsMetadataManager. The second test does not modify the contents of segments in the sqlSegmentsMetadataManager and does not affect the assertions in the first test. We can run all the tests just on a single sqlSegmentsMetadataManager to speed up the tests in the test class SqlSegmentsMetadataManagerEmptyTest. The runtime can decrease from 2.997 s to 2.285 s after applying the changes when run on our own machine.

Release note

---

Key changed/added classes in this PR

• Test class SqlSegmentsMetadataManagerEmptyTest is changed in this PR.

---

This PR has:

• been self-reviewed.
  • using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
• added documentation for new or modified features or behaviors.
• a release note entry in the PR description.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added or updated version, license, or notice information in licenses.yaml
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• added integration tests.
• been tested in a test Druid cluster.

[kfaraz]

This change doesn't really seem to be needed. It just seems to be making everything static to avoid creation of a few objects. In and of itself, the change here is fairly simple and is okay to be be merged but in my opinion, it sets a bad precedent. A similar PR #15533 has been recently merged.

These are my primary concerns:

• Making everything static just to save on some object creation in tests doesn't seem like much of an approach. We should have a better reason of why a field should be static or not.
• We do want the target object (sqlSegmentsMetadataManager) to be freshly created in each test and thus be non-static. The other object jsonMapper is already being initialized just once. If and when more tests are added to this class, these fields would have to be reverted to non-static anyway.
• The improvement of 2.9s to 2.3s in a single test doesn't justify this PR.
• Test improvements should not come at the cost of readability.
• The user @TestBoost has been recently created and looks suspiciously like a bot. 🙂

@TestBoost , please share some more information here. If you have other similar changes, it would be preferable to create a single PR containing such improvements.

[TestBoost]

Hi,

To clarify, this is not a bot account. We recently developed a new tool for transforming test classes to speed up testing by reducing the number of times test fixtures (setup and teardown methods) are run, by essentially making them run at the class level instead of at the method level (between every test method). Using our tool, we were able to transform other test classes within this project, including the one from #15533, but we had the mindset of sending pull requests for a few at a time as to not overwhelm. If you would prefer that we send many or all at once, we can try that.

If you have any feedback on this approach or what we could do better, we would be happy to change things accordingly.

[kfaraz]

Thanks for the response, @TestBoost .

• Since all the changes seem small and are in the same vein, it would be better to have a single PR that is updating all the tests in a single module (e.g. the module touched in this PR is server), if not the entire Druid project.
• It would be nice to know what is the overall time benefit that you are gaining by these changes.
• For the current PR in particular, it makes sense for derbyConnectorRule and jsonMapper to be static but not the the test target sqlSegmentsMetadataManager as we explicitly want it to be freshly created in every test. This is true even when the metadata manager state doesn't seem to be altered in a particular test.
• For only create used and unused segments once to make the test faster #15533, I have left a comment on that PR itself.

[TestBoost]

Thank you very much for pointing out!

We don't find any other tests like this in server module and other modules in druid as of now. But there are other test classes that require splitting tests that utilize the same variables/resources into different test classes to make tests faster. I don't know if it's suitable. And I also applied the changes as you said in this pull request.

[kfaraz]

@TestBoost , taking another look at this test class, I realize that it might not be needed at all. I have created #16044 to merge this test into the bigger one instead.

[TestBoost]

Thank you so much for letting me know!!! I can close this pull request now.