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

[GOBBLIN-2160] added some unit tests for gobblin temporal module #4059

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pratapaditya04
Copy link
Contributor

@pratapaditya04 pratapaditya04 commented Sep 24, 2024

Dear Gobblin maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

Description

  • Here are some details about my PR, including screenshots (if applicable):
    Added Unit tests for JobStateUtils and AbstractNestingExecWorkflowImpl classes in Gobblin-temporal module

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Comment on lines +137 to +138
@VisibleForTesting
public static List<Integer> consolidateSubTreeGrandChildren(final int numSubTreesPerSubTree,
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that you added @VisibleForTesting and changed modifier to public for testing this method. However, this change is not required if we follow the same package structure for the test class and we can access protected methods without exposing them publicly. We can revert the modifier change and place the test class in the same package instead.

* limitations under the License.
*/

package org.apache.gobblin.temporal.ddm.workflow.impl;
Copy link
Contributor

Choose a reason for hiding this comment

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

update package to org.apache.gobblin.temporal.util.nesting.workflow

}

@Test(expectedExceptions = AssertionError.class)
public void testPerformWorkload_LaunchesChildWorkflows() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you've covered some scenarios, like handling an empty WorkSpan and testing the pause duration. However, the current tests don't fully capture the functionality(creating child workflows, subtrees handling, result calculation and edge cases) of the performWorkload method. It would be useful to add overall coverage for a method when we are adding tests for it, this ensures that the tests fulfill the original intent of thoroughly validating the behavior of missing tests. This would also help avoid the need for additional work on this method again later, unless we are making changes to it.

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