Refactor Shenandoah parser #190
Conversation
Codecov Report
@@ Coverage Diff @@
## feature/enhance-shenandoah-datareader #190 +/- ##
==========================================================================
+ Coverage 62.48% 62.6% +0.11%
+ Complexity 1290 1287 -3
==========================================================================
Files 143 143
Lines 8189 8174 -15
Branches 1317 1307 -10
==========================================================================
Hits 5117 5117
+ Misses 2578 2567 -11
+ Partials 494 490 -4
Continue to review full report at Codecov.
|
ghost
force-pushed
the
feature/enhance-shenandoah-datareader
branch
from
00f4745 to
cb6bef8
Compare
There was a problem hiding this comment.
I like very much, how short DataReaderShenandoah is! Other parsers usually needed many more lines to achieve good coverage of the files.
I also like the test coverage. Now every last event is covered.
I have found few really small things. Could you have a look into them, please?
| @@ -88,7 +94,10 @@ public GCModel read() throws IOException { | |||
| model.setFormat(GCModel.Format.RED_HAT_SHENANDOAH_GC); | |||
|
|
|||
| Stream<String> lines = new BufferedReader(in).lines(); | |||
There was a problem hiding this comment.
No need to wrap "in" in a BufferedReader. It is a LineNumberReader, which is a subclass of BufferedReader.
| event = line.contains("Concurrent") ? new ConcurrentGCEvent() : new GCEvent(); | ||
| AbstractGCEvent.Type type = AbstractGCEvent.Type.lookup(withHeapMatcher.group(WITH_HEAP_EVENT_NAME)); | ||
| event.setType(type); | ||
| setPauseAndTimestamp(event, withHeapMatcher.group(WITH_HEAP_DURATION), withHeapMatcher.group(WITH_HEAP_TIMESTAMP)); | ||
| addHeapDetailsToEvent(event, withHeapMatcher.group(WITH_HEAP_MEMORY)); | ||
| } else { | ||
| getLogger().warning("Found line that has no match:" + line); |
There was a problem hiding this comment.
Usually, I add the line number to the log warnings, because this makes finding the offending line in the log easier. You might be able to use in.getLineNumber() (might not work here, because of the usage of the streams - I don't have any experience, there).
If it is difficult to add the line number - no problem. The warning also helps without the line number.
| assertThat("amount of STW GC pause types", model.getGcEventPauses().size(), is(2)); | ||
| assertThat("amount of STW Full GC pause types", model.getFullGcEventPauses().size(), is(0)); | ||
| assertThat("amount of concurrent pause types", model.getConcurrentEventPauses().size(), is(3)); | ||
| assertThat("total pause time", model.getPause().getSum(), is(0.001318)); |
There was a problem hiding this comment.
I think, tests for "double" values should usually be made using Matchers.closeTo() (examples can be found in TestDataReaderSun1_8_0G1). The reason is, that the representation of double values is not exact and can lead to tests failing randomly.
Task list for this pull request:
-Xlog::ShenandoahSampleDateTimeStamps.txt:time,level,tags