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

feat(queryAPI): add filter for instances with jobs retrying in java API #4920

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PHWaechtler
Copy link
Contributor

related to #4904

@PHWaechtler PHWaechtler added the scope:core-api Changes to the core API: engine, dmn-engine, feel-engine, REST API, OpenAPI label Feb 4, 2025
@PHWaechtler PHWaechtler self-assigned this Feb 4, 2025
@PHWaechtler PHWaechtler force-pushed the 4904-filter-instances-with-jobs-retrying-java-api branch 4 times, most recently from 66a140c to 9462e8e Compare February 5, 2025 07:42
@PHWaechtler PHWaechtler added ci:rest-api Runs extra builds for the REST API (currently only WLS compatibility builds). and removed scope:core-api Changes to the core API: engine, dmn-engine, feel-engine, REST API, OpenAPI labels Feb 5, 2025
@PHWaechtler PHWaechtler force-pushed the 4904-filter-instances-with-jobs-retrying-java-api branch from 9462e8e to 0728abd Compare February 12, 2025 09:48
@PHWaechtler PHWaechtler requested a review from psavidis February 12, 2025 14:02
@psavidis
Copy link
Contributor

psavidis commented Feb 13, 2025

Review

1. AssertJ

  • 🔧 For the sake of uniformity and Testing Best Practices (favouring the DSL explicitness to avoid ambiguity), AssertJ also for the equality check. I've collected the suggestions here to avoid repeating the same point over and over again:
    • #1, #2, #3, #4,
    • #5 - This point changes also the id used for the assertion

2. Formatting
Unfortunately most classes in the project are not formatted entirely. The changes in the HistoricProcessInstanceTest span across the whole class due to the formatter. This makes the discovery of the changes to the file hard to track.

❓ Can we get rid of the formatting changes to the file so that only the changes for this feature show up in the PR? I assume the real change is only the addition of testHistoricProcessInstanceQueryWithJobsRetrying ?

3. Review Testing

  • ✔️ When the new criterion subquery is changed (e.g changing the WHERE condition to equals), the test fails

  • ✔️ The new test utilises a query with the new filter which covers all cases for job statuses:

    • Jobs started
    • A failed job
    • A successfully completed job
  • New SQL Condition Coverage: The newly added subquery has two conditions on EXCEPTION_MSG and the EXCEPTION_STACK_ID. Since the test uses the FailingDelegate to simulate the job failure, would it make sense to control the FailingDelegate#message variable and add a test for :

    • A Failing Delegate with Null message
    • A Failing Delegate with a ProcessEngineException with the empty constructor

4. CI
Since the changes add the new criterion to the Query API and the change involves changing the SQL, building the effective PR changes against the following two profiles will increase our confidence :

  • ci:default-build
  • ci:all-db

// then
assertEquals(1, queryWithJobsRetrying.count());
assertEquals(1, queryWithJobsRetrying.list().size());
assertEquals(instanceWithRetryingJob.getId(), queryWithJobsRetrying.singleResult().getRootProcessInstanceId());
Copy link
Contributor

Choose a reason for hiding this comment

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

This suggestion contains the following two points:

  • 🔧 Perhaps this is a good example for favouring AssertJ over JUnit assertions; i got slightly confused what this assertion wants to achieve
  • ❓At this point, the test after failing one job and completing successfully the other would assert that: when you query for retrying jobs you should find the instance with retrying jobs. Why not use the Job Id for that as like the rest of the jobs? The rootProcessInstanceId happens to be the same but semantically communicates something slightly different.
Suggested change
assertEquals(instanceWithRetryingJob.getId(), queryWithJobsRetrying.singleResult().getRootProcessInstanceId());
assertThat(queryWithJobsRetrying.singleResult().getId()).isEqualTo(instanceWithRetryingJob.getId());

Copy link
Contributor Author

@PHWaechtler PHWaechtler Feb 14, 2025

Choose a reason for hiding this comment

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

when you query for retrying jobs you should find the instance with retrying jobs.

I am expecting to find instances because we are querying for instances that are associated with retrying jobs (not the jobs themselves). Is this Job ID a field of the instance you're referring to? I am not seeing it. Or did you mean instance ID? I assumed that was the same as rootInstanceID just by looking at the values/name, didnt realise there was additional logic behind that. I have changed it to just the ID of the instance. Lmk if it should be pointing to some job though

@PHWaechtler
Copy link
Contributor Author

PHWaechtler commented Feb 14, 2025

1. AssertJ

  • 🔧 For the sake of uniformity and Testing Best Practices (favouring the DSL explicitness to avoid ambiguity), AssertJ also for the equality check.

I would also prefer AssertJ, but the rest of the test cases use JUnit. For consistency I then also used the same style assertions as in the other test cases, should I rewrite the rest of the test cases in this class too? Or can we open a follow up ticket for that?

2. Formatting Unfortunately most classes in the project are not formatted entirely. The changes in the HistoricProcessInstanceTest span across the whole class due to the formatter. This makes the discovery of the changes to the file hard to track.

Yeah I brought this up a couple of times as I was susprised to see the lack of formatting, this will always bloat PRs in annoying ways. I am considering formatting files that a PR will touch first in a separate commit before adding changes, but even that is kinda inconvenient when working as you cant always predict all files you're about to change. I've not found another workaround if we dont want to fix the formatting in the codebase.

❓ Can we get rid of the formatting changes to the file so that only the changes for this feature show up in the PR? I assume the real change is only the addition of testHistoricProcessInstanceQueryWithJobsRetrying ?

If we undo the formatting changes the file remains unformatted though, introducing untidy code and not fixing the current formatting issues. I feel like that will just making be the issue worse 🤔

  • New SQL Condition Coverage: The newly added subquery has two conditions on EXCEPTION_MSG and the EXCEPTION_STACK_ID. Since the test uses the FailingDelegate to simulate the job failure, would it make sense to control the FailingDelegate#message variable and add a test for :

    • A Failing Delegate with Null message
    • A Failing Delegate with a ProcessEngineException with the empty constructor

Yes, I oriented the conditions on the existing Job Query for this type of scenario, covering the same fields. I do not know how to trigger the two scnearios you've listed, do you have a pointer to where similar behaviour has been used in a test case so I can refer to that for implemenation?

Update:
Regarding the failing delegate test cases, as discussed on zoom we added one test case where the exception messsage is null for extra coverage. As for the case that the stacktrace is null, we did not add an extra test case because we discovered it would be too much effort to create such an exception. The combination of the two (exception method and stack trace check) is already used in the job queries however, giving more confidence.

4. CI Since the changes add the new criterion to the Query API and the change involves changing the SQL, building the effective PR changes against the following two profiles will increase our confidence :

  • ci:default-build
  • ci:all-db

Sounds good will add once I added the changes.

@PHWaechtler PHWaechtler added ci:all-db Runs the builds for all databases. ci:default-build Runs the builds that have no explicit trigger (e.g. different history levels). and removed ci:rest-api Runs extra builds for the REST API (currently only WLS compatibility builds). labels Feb 14, 2025
@PHWaechtler PHWaechtler force-pushed the 4904-filter-instances-with-jobs-retrying-java-api branch from 0728abd to 0f20c65 Compare February 14, 2025 09:23
@PHWaechtler PHWaechtler force-pushed the 4904-filter-instances-with-jobs-retrying-java-api branch from 0f20c65 to 154b6cc Compare February 14, 2025 11:07
@PHWaechtler
Copy link
Contributor Author

A summary of the changes from code review:

AssertJ
Adjusted the assertions in the new test case but left the others as is. Do we have a ticket for this kind of maintenance, or should we open a follow up? It looks like there would be a lot of tests to adjust.

Formatting
Discussed our dislike of the current state and I will be more wary to try to move the auto formatting to separate commits in following PRs. Plus might push for this topic on a team level again, but its outside of the scope of this ticket.

SQL condition coverage for null exception msg/stack trace
We discussed this on zoom and tried it, but it seems the effort to try to create this scenario is not worth it. We also discussed that this instance query should in behaviour match the existing job query for jobs with exceptions, the existing job query has the same sql, giving us more confidence.

CI
Added the additional labels ci:default-build and ci:all-db

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-db Runs the builds for all databases. ci:default-build Runs the builds that have no explicit trigger (e.g. different history levels).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants