Better logging #88
Better logging #88
Conversation
szeiger
commented
Jun 28, 2019
szeiger
commented
- Make better use of the "new" test interface
- Improved logging options, prompted by Show MiMa logs at the end of testAll in case of MiMa failures scala/scala#8178
- "Test run started/finished" messages include class name. - More useful verbosity settings with a new option to show only the (now more useful) "Test run finished" messages. - Optional summary with more details.
There was a problem hiding this comment.
It's nice to see the project getting some love! Can you share an example of the default output when all the tests are passing and when one is failing? It's an area that's caused issues in this project in the past, so I wanted to make sure it's what users are expecting. Thanks!
| ju.addListener(ed); | ||
| if (runner.runListener != null) ju.addListener(runner.runListener); | ||
|
|
||
| Map<String, Object> oldprops = settings.overrideSystemProperties(); |
There was a problem hiding this comment.
Can you add a comment explaining why this is necessary? I'm not exactly sure what it's doing, but it seems a little hackish and I wonder if there isn't a better way
There was a problem hiding this comment.
It's more than a little hackish but this is all old code that I only moved around. No actual changes here.
| @@ -151,12 +158,16 @@ void logTo(RichLogger logger) { | |||
| private void postIfFirst(AbstractEvent e) | |||
| { | |||
| e.logTo(logger); | |||
| if(reported.add(e.fullyQualifiedName())) handler.handle(e); | |||
| if(reported.add(e.fullyQualifiedName())) { | |||
There was a problem hiding this comment.
I generally would put a space after if, for, etc. Can we consider changing throughout this PR?
There was a problem hiding this comment.
These extra spaces always look wrong to me. Most of the code here is probably without spaces (unless someone else rewrote it in the mean time). My style changed over time to put opening braces at the end of the line even in Java and this is inconsistent in the codebase, too. This could be made consistent in a separate PR but honestly I'm not too bothered by it.
| } | ||
|
|
||
| @Override | ||
| public String done() { | ||
| return ""; | ||
| // Can't simply return the summary due to https://github.com/sbt/sbt/issues/3510 | ||
| if(!used) return ""; |
There was a problem hiding this comment.
It'd be nice to use multi-line curly braces:
if (!used) {
return "";
}
There was a problem hiding this comment.
This just looks convoluted to me. I know many people like this style but I have no idea why.
The default output hasn't changed at all and neither has the meaning of the existing options. The only visible change is when using The "Test run" lines didn't have any test name in them before. Now they contain the name that was seen by sbt as the test case (in this case With the new This new mode only makes sense with the addition of the test names in these lines. |
|
For anyone looking at the code, the nomenclature will be confusing. I didn't make any changes in this regard and I also think it is good on the user-facing side but sbt uses confusing names in the test interface.
|
