-
Notifications
You must be signed in to change notification settings - Fork 569
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
Add QA tests that verify CreateProcessInstanceWithResult responses #11993
Conversation
These two cases failed recently due to a bug introduced with batch processing [1]. The situation happened because multiple commands were being processed and responded in the same batch, while the batch processing assumed only a single response would be writen. As this affected behavior visible to users, it makes sense to introduce qa integration tests that show case that the CreateProcessInstanceWithResult can actually respond when the process instance is completed by another user command: - complete job - publish mesage See: [1] #11848
Hi @berkaycanbc. There is no issue tracking this pull request, as the related issue is already closed when the bug was fixed. But, I still think it makes sense to add these test cases. Can you take a look when you have time? Let me know if you have any questions, or if you want to have a look at them together. |
Test Results1 015 files + 1 1 015 suites +1 1h 52m 15s ⏱️ - 4m 14s Results for commit 892135a. ± Comparison against base commit 6c36841. This pull request removes 699 and adds 613 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
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.
I also observed that previous related PR has tests that only cover general use case. Really nice that you added tests for actual cases that are observed by users. 🚀
❓ One additional question: do we have other cases where we can complete create process instance command? If so, it might be good to add test cases for them as well.
Approving it as I expect these are all the cases available.
I had not seen this before, but the BrokerClassRuleHelper is used in our QA tests which provides some nice helper methods, like `.getBpmnProcessId()` which provides a unique process id for each test case. This helps isolate the tests a bit more.
This adds test cases for each of the ways that a user can complete the process instance that the user is awaiting a result for, except for those added previously (complete job and publish message). Note that the CreateProcessInstanceWithResult RPC explicitly only responds when the process instance **completes**. So we do not have to add test cases where the process instance **terminates**.
👍 To be honest, I hadn't given much thought to this until I read this review comment. As a result, I added 892135a. Thanks for this great review hint! 🚀 @berkaycanbc Can you have another look? |
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.
@korthout Really nice that we covered all cases to complete a process! 🚀
Also, these test cases were clear references for me to see possible ways to complete a process. Thanks!
bors merge |
Build succeeded: |
Successfully created backport PR for |
Successfully created backport PR for |
Description
These two cases failed recently due to a bug introduced with batch processing [1].
As this affected behavior visible to users, it makes sense to introduce QA integration tests that showcase that the
CreateProcessInstanceWithResult
can actually respond when the process instance is completed by another user command.Related issues
relates to #11848
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3
) to the PR, in case that fails you need to create backports manually.Testing:
Documentation:
Other teams:
If the change impacts another team an issue has been created for this team, explaining what they need to do to support this change.
Please refer to our review guidelines.