-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: master
Are you sure you want to change the base?
feat(queryAPI): add filter for instances with jobs retrying in java API #4920
Conversation
66a140c
to
9462e8e
Compare
9462e8e
to
0728abd
Compare
Review1. AssertJ
2. Formatting ❓ 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 3. Review Testing
4. CI
|
engine/src/test/java/org/camunda/bpm/engine/test/history/HistoricProcessInstanceTest.java
Outdated
Show resolved
Hide resolved
engine/src/test/java/org/camunda/bpm/engine/test/history/HistoricProcessInstanceTest.java
Outdated
Show resolved
Hide resolved
engine/src/test/java/org/camunda/bpm/engine/test/history/HistoricProcessInstanceTest.java
Outdated
Show resolved
Hide resolved
engine/src/test/java/org/camunda/bpm/engine/test/history/HistoricProcessInstanceTest.java
Outdated
Show resolved
Hide resolved
// then | ||
assertEquals(1, queryWithJobsRetrying.count()); | ||
assertEquals(1, queryWithJobsRetrying.list().size()); | ||
assertEquals(instanceWithRetryingJob.getId(), queryWithJobsRetrying.singleResult().getRootProcessInstanceId()); |
There was a problem hiding this comment.
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? TherootProcessInstanceId
happens to be the same but semantically communicates something slightly different.
assertEquals(instanceWithRetryingJob.getId(), queryWithJobsRetrying.singleResult().getRootProcessInstanceId()); | |
assertThat(queryWithJobsRetrying.singleResult().getId()).isEqualTo(instanceWithRetryingJob.getId()); |
There was a problem hiding this comment.
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
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?
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.
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 🤔
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:
Sounds good will add once I added the changes. |
0728abd
to
0f20c65
Compare
0f20c65
to
154b6cc
Compare
A summary of the changes from code review: AssertJ Formatting SQL condition coverage for null exception msg/stack trace CI |
related to #4904