Std out sink to use event model #599
Conversation
Signed-off-by: Taylor Gray <[email protected]>
Signed-off-by: Taylor Gray <[email protected]>
Signed-off-by: Taylor Gray <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #599 +/- ##
============================================
- Coverage 91.74% 91.49% -0.26%
+ Complexity 571 570 -1
============================================
Files 72 72
Lines 1744 1751 +7
Branches 144 145 +1
============================================
+ Hits 1600 1602 +2
- Misses 113 114 +1
- Partials 31 35 +4
Continue to review full report at Codecov.
|
| private final Record<String> TEST_RECORD_1 = new Record<>(TEST_DATA_1); | ||
| private final Record<String> TEST_RECORD_2 = new Record<>(TEST_DATA_2); | ||
| private final List<Record<String>> TEST_RECORDS = Arrays.asList(TEST_RECORD_1, TEST_RECORD_2); | ||
| private final Map<String, Object> TEST_DATA_1 = new HashMap<>(); |
There was a problem hiding this comment.
I recommend that you make this a non-final and initialize it in the setup. As it is right now, authors will need to clear the map in-between tests.
The same goes for TEST_DATA_2 and TEST_RECORDS.
Signed-off-by: Taylor Gray <[email protected]>
| @@ -32,7 +32,7 @@ jacocoTestCoverageVerification { | |||
| violationRules { | |||
| rule { //in addition to core projects rule | |||
| limit { | |||
| minimum = 0.90 | |||
| minimum = 0.89 | |||
There was a problem hiding this comment.
Let's keep the threshold as is. There is nothing in this PR that requires us to lower it.
There was a problem hiding this comment.
You would think so, but the coverage is now .89 after this PR
There was a problem hiding this comment.
I ran your changes locally addressing the Junit issue and looking at the code coverage results from the build. It is possible to achieve .90. Let's keep it at .90.
There was a problem hiding this comment.
I had tried this yesterday and it did fix it. Had bumped it back to 0.90 for the next revision. Interesting little thing there with the 2 different versions of Junit. Thanks for catching it!
| import org.junit.Test; | ||
| import org.junit.jupiter.api.BeforeEach; |
There was a problem hiding this comment.
You are using two different version of Junit now. I believe this is what is impacting your test coverage.
There was a problem hiding this comment.
Good catch! It's a subtle difference.
There was a problem hiding this comment.
Thanks for making it support both! I have one comment on variable names and then I'll be ready to approve. It looks like the 90% coverage is working again, correct?
Yep .90 is working again
Signed-off-by: Taylor Gray <[email protected]>
| private final Record<String> TEST_RECORD_1 = new Record<>(TEST_DATA_1); | ||
| private final Record<String> TEST_RECORD_2 = new Record<>(TEST_DATA_2); | ||
| private final List<Record<String>> TEST_RECORDS = Arrays.asList(TEST_RECORD_1, TEST_RECORD_2); | ||
| List<Record<Object>> TEST_RECORDS; |
There was a problem hiding this comment.
This field and the variables in setup should be renamed to standard Java field and variable conventions.
Signed-off-by: Taylor Gray <[email protected]>
Signed-off-by: Taylor Gray <[email protected]>
Description
Updated stdout sink to use the Event model
Issues Resolved
Closes #585
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.