Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

only create used and unused segments once to make the test faster #15533

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

TestBoost
Copy link
Contributor

@TestBoost TestBoost commented Dec 9, 2023

This pull request is trying to make test run faster. When run on our own machine, the test runtime can just from 3.583 s to 2.939 s after applying the change.

Description

In this pull request, we try to only create the task, used segments and unused segments once. In the original setup method, it creates a task and create the used segments and unused segments before every test runs.

However, the first test testRetrieveUsedSegmentsAction only tries to assert the retrieved used segments are the same as the original created used segments. The second test testRetrieveUnusedSegmentsAction tries to assert the retrieved unused segments are the same as the original created unused segments. We do not need create the used and unused segments every time. Thus, we can just create them only once before all tests run. Another way can be create the used segments for the first test only and create the unused segments for the second test only.

Besides, when we inspect the code inside, action.perform will just make a copy of used / unused segments and it does not modify the existing fields in this test class.

Release note


Key changed/added classes in this PR
  • RetrieveSegmentsActionsTest is changed to make it run faster.

This PR has:

  • been self-reviewed.
  • 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.

Copy link
Contributor

@AmatyaAvadhanula AmatyaAvadhanula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @TestBoost. LGTM

@AmatyaAvadhanula AmatyaAvadhanula merged commit 85af2c8 into apache:master Dec 12, 2023
83 checks passed
Copy link
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of making everything static, the right solution here would have been as follows:

  • Create and process expectedUsedSegments only in the method testRetrieveUsedSegmentsAction as a local variable
  • Create and process expectedUnusedSegments only in the method testRetrieveUnusedSegmentsAction as a local variable
  • Do not make the fields task and actionTestKit static as we do want to start fresh in every test.

cc: @AmatyaAvadhanula , @TestBoost

@LakshSingla LakshSingla added this to the 29.0.0 milestone Jan 29, 2024
@TestBoost
Copy link
Contributor Author

Sorry for the late reply and I just came back from holiday. I tried to make the changes as you said. However, I found there is another test testRetrieveUnusedSegmentsActionWithNowUsedLastUpdatedTime in this test class which also needs expectedUnusedSegments. We need to create expectedUsedSegments in the method testRetrieveUsedSegmentsAction and create expectedUnusedSegments in the method testRetrieveUnusedSegmentsAction and testRetrieveUnusedSegmentsActionWithNowUsedLastUpdatedTime. This makes the test even slower than the original one. The original test takes 3.583 s. After applying this commit, it takes 2.939 s. And after making expectedUsedSegments and expectedUnusedSegments as local variables, it takes 4.761 s.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants