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

Increase user task client test coverage #19420

Merged
merged 3 commits into from
Jul 3, 2024
Merged

Conversation

tmetzke
Copy link
Member

@tmetzke tmetzke commented Jun 17, 2024

Description

Adds unit tests to the Java client to ensure the user task-related commands for assigning, completing, unassigning, and updating tasks work as expected.

Related issues

closes #19374

@tmetzke tmetzke self-assigned this Jun 17, 2024
@github-actions github-actions bot added the component/zeebe Related to the Zeebe component/team label Jun 17, 2024
@tmetzke
Copy link
Member Author

tmetzke commented Jun 17, 2024

Recreating #19382 because I merged that too quickly.

Copy link
Member

@koevskinikola koevskinikola left a comment

Choose a reason for hiding this comment

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

👍 This one is already approved, but we don't want to merge it before #19333.

@tmetzke tmetzke force-pushed the 18256-job-activation-rest-client branch from 306c7ce to a01e217 Compare June 24, 2024 12:28
@tmetzke tmetzke linked an issue Jun 28, 2024 that may be closed by this pull request
@tmetzke tmetzke force-pushed the 19374-user-task-client-tests branch 2 times, most recently from 3f4e904 to b229723 Compare June 28, 2024 19:30
@tmetzke tmetzke force-pushed the 18256-job-activation-rest-client branch from 9cbadfb to 127ba24 Compare June 30, 2024 09:02
@tmetzke tmetzke force-pushed the 19374-user-task-client-tests branch from b229723 to c9e9c34 Compare June 30, 2024 09:03
@tmetzke tmetzke force-pushed the 18256-job-activation-rest-client branch from 127ba24 to c7d99ff Compare July 1, 2024 05:19
@tmetzke tmetzke force-pushed the 19374-user-task-client-tests branch 2 times, most recently from bd3b256 to 14f3dc3 Compare July 1, 2024 06:16
@tmetzke tmetzke force-pushed the 18256-job-activation-rest-client branch from 663be93 to 19ce6a4 Compare July 1, 2024 17:10
@tmetzke tmetzke force-pushed the 19374-user-task-client-tests branch from 14f3dc3 to a90dede Compare July 1, 2024 17:11
Base automatically changed from 18256-job-activation-rest-client to main July 1, 2024 18:05
@tmetzke tmetzke enabled auto-merge July 1, 2024 18:12
@korthout korthout self-requested a review July 2, 2024 07:41
Copy link
Member

@korthout korthout left a comment

Choose a reason for hiding this comment

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

Great stuff @tmetzke 🚀

🔧 I just have a few suggestions

LGTM 👍

void shouldRaiseExceptionOnError() {
// given
gatewayService.errorOnRequest(
String.format(RestGatewayService.URL_USER_TASK_ASSIGNMENT, 123L),
Copy link
Member

Choose a reason for hiding this comment

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

🔧 If you happen to move the template strings into a separate class, you could provide a method to get the URL. It can be implemented with this same call to String.format, but provide more clarity to the reader. For example, this could allow method chaining to clarify intent (although perhaps a bit overengineered):

gatewayService.errorOnRequest(
    GatewayServicePaths.resolveUrl(resource: USER_TASK).key(123L).assignment()
    () -> new ProblemDetail().title("Not Found").status(404));

💭 This also makes me think that it may be useful to allow passing a specific request method to the errorOnRequest, so tests have more control over the specific request that's mocked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting thought, I'll give this a try 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I played around with that for a bit but I think we're really over-engineering this for what we currently need. I settled for a "middle ground", making the URL templates internal and providing respective static getters that clarify what arguments are required to build a proper URL for each endpoint.

Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

I like it! 👍

@tmetzke tmetzke added this pull request to the merge queue Jul 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 2, 2024
Adds unit tests to the Java client to ensure the user task-related commands for
assigning, completing, unassigning, and updating tasks work as expected.
Moves the REST URLs into a dedicated class that takes care of building them correctly.
The URL templates are internal and there is a getter for each of them, clarifying which
arguments are needed to build a proper URL.
In the Java client, the user task assignment command doesn't allow providing
a null assignee. The respective test class is extended to ensure this behavior.
@tmetzke tmetzke force-pushed the 19374-user-task-client-tests branch from a90dede to b44381f Compare July 3, 2024 08:26
@tmetzke
Copy link
Member Author

tmetzke commented Jul 3, 2024

For the sake of progress, I'll go ahead and merge here. Any further suggestions can go into follow-up issues.

@tmetzke tmetzke added this pull request to the merge queue Jul 3, 2024
Merged via the queue into main with commit 8cb0550 Jul 3, 2024
57 checks passed
@tmetzke tmetzke deleted the 19374-user-task-client-tests branch July 3, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/zeebe Related to the Zeebe component/team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase user task test coverage in the Java client
3 participants