Log PROCESS_INSTANCE_CREATION records with start instructions #434
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
remcowesterhoud
requested changes
Jul 5, 2022
filters/src/test/java/io/camunda/zeebe/process/test/filters/logger/RecordStreamLoggerTest.java
Outdated
Show resolved
Hide resolved
filters/src/test/java/io/camunda/zeebe/process/test/filters/logger/RecordStreamLoggerTest.java
Outdated
Show resolved
Hide resolved
korthout
force-pushed
the
korthout-411-log-start-instructions
branch
from
90c4e04 to
ef24358
Compare
The logRecords method could never be used to test because it only has side effects (printing to log), but internally it works by building up a String with a StringBuilder. By extracting this string building into a separate method, its easy to test the produced String that will be logged.
The logStartInstructions is a direct copy of the CompactRecordLogger implementation in Zeebe, with 1 exception: - removed the spaces around the resulting string to fit better into the result
Uses the ImmutableRecord builder to create a record, stringify the record for logging and then asserts the resulting string. I've chosen to test 2 cases: no instructions, 2 instructions. 1 instruction would be an untested edge case, but it doesn't really matter IMO.
By using a parameterized test, its easier to expand the tests. Simply add another Argument with a record and what the log should contain.
These commas look strange in the printed logs. They shouldn't be necessary
This time for jobs.
korthout
force-pushed
the
korthout-411-log-start-instructions
branch
from
ccee8d4 to
723fe0e
Compare
|
@remcowesterhoud, I've made the requested change. I feel like the error logging is a very important part of ZPT, and we should probably focus on improving the test coverage of this logging. That's why I've gone back and tried to improve the tests that I originally added. It should now be easier to expand them for other records. I've also applied the comma fix for Job record logging, but I haven't changed it for all records. I don't want to mess too much with it without having tests in place. We should consider making that a bigger issue |
|
@remcowesterhoud Would be cool to get this merged before creating |
remcowesterhoud
approved these changes
Jul 12, 2022
There was a problem hiding this comment.
Sorry for taking so long to review, thanks for the fix @korthout 🏆
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Expands the RecordLogger to also log the start instructions of
PROCESS_INSTANCE_CREATIONrecords.Also adds easier testing to the RecordStreamLoggerTest.
Related issues
closes #411
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
Testing:
Documentation: