Skip to content
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

Make compact reporter print skips/info/errors last. #982

Closed
wants to merge 2 commits into from

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Feb 10, 2019

When running a large test suite (e.g. for the Flutter repo), having skips/errors/informational messages printed as they come becomes difficult to track:

  • If you have a failed test, it might get lost in your console's buffer or require you to do lots of scrolling to find it
  • This is particularly challening for us on CI reports, since that only shows us the tail end of the log and requires you to download and open the log in another program to see the full history
  • Even opening the test results in a separate file is tough if you have multiple errors - you're searching for things like -1: in the file, and may find that errors came thousands of lines apart and are difficult to correlate.

This patch makes it so that the compact reporter will defer printing skipped test message, info messages (print in a test), and error messages until the end of execution. It also will dump them out if a user hits Ctrl+C during a test (so if you start to see a bunch of failures and just want to bail out to see what's up without continuing).

I'm not sure of the impact this might have for web. If this is deemed destructive for current users, then I'd be open to moving it to a new reporter.

/cc @goderbauer @gspencergoog (we were discussing that this is a problem the other day)
/cc @jonahwilliams

@jonahwilliams
Copy link
Contributor

I greatly endorse this product and/or service.

@goderbauer
Copy link
Contributor

I would buy the premium testrunner service just to get this feature!

@kevmoo kevmoo requested a review from grouma February 10, 2019 04:34
@grouma
Copy link
Member

grouma commented Feb 10, 2019

I think this is an improvement as I too occasionally struggle to hunt down error logs for large test suites. This will have to be rolled as a breaking change unfortunately. I hope it will be minimally destructive to our internal customers. I also have concerns with the change to the information messages. With this PR you lose the association of message to test. Is there a particular reason you wanted to delay those logs?

Finally, as an alternative, have you played around with consuming the json reporter results in the flutter test runner to essentially create a custom flutter reporter? I think that approach is more flexible but I'm not sure if it's necessary.

@jonahwilliams
Copy link
Contributor

A less breaking option might be to leave the existing formatter as is mostly, and allow an optional summary report at the end.

@dnfield
Copy link
Contributor Author

dnfield commented Feb 10, 2019

information messages

I was considering printing out the source file/line/column for information messages, or just putting them back in with the rest of the output. The problem with keeping them with the rest of the output is you still end up with lots of lines of output that are easy to lose track of as the suite is running.

json reporter

I did look at this as well. I had a little trouble making sure I was getting all the test cases, and it was slightly slower than modifying this. I also suspected this is something that impacts a lot of people using test, and that many people would benefit from this solution. If this really is a big breaker for people though, I'd be happy to see it either as an alternative reporter or as a wrapper around the json output.

@dnfield
Copy link
Contributor Author

dnfield commented Feb 10, 2019

Actually, I've gotten my json wrapper working about as well as this. I also like the idea of being able to potentially send test information for analysis to spanner or something.

If you think this could be valuable with some tweaks I'm happy to do so.

@grouma
Copy link
Member

grouma commented Feb 11, 2019

Let me attempt an internal roll to see how problematic this change may be. The JSON reporter approach is a fine alternative if this change proves to be troublesome.

@jakemac53
Copy link
Contributor

A less breaking option might be to leave the existing formatter as is mostly, and allow an optional summary report at the end.

I like the idea of an optional summary report argument, we could add this to all the reporters then? It doesn't feel right to do this just in a specific reporter (unless that was the entire purpose of the reporter, but the compact reporter is a more general one).

@natebosch
Copy link
Member

I'm strongly against deferring print output in the default reporter. The default reporter is optimized for interactive test running. I'm in favor of creating a new reporter optimized for CI and other non-interactive scenarios.

@grouma
Copy link
Member

grouma commented Feb 11, 2019

@jakemac53 @natebosch and I chatted a bit more. We agree the best approach is to provide a new optional --summary flag. This would print all names of failing tests at the end. This would work with all existing reporters although the JSON reporter would ignore it. We can create a new reporter for CIs that is essentially silent. Would this work for you? Do you think this is something you can do?

@dnfield
Copy link
Contributor Author

dnfield commented Feb 11, 2019

Yes, that would be fine. Honestly the compact reporter would still be messy (in that I would expect the compact reporter to never overflow my terminal buffer, no matter how many errors/info/skips are present in the test), but if it breaks people to fix that then that's a shame.

I'll take a look at adding a --summary flag - I imagine it wouldn't be much more work than what I've done here anyway. We could potentially still have it for JSON and just output a JSON summary, right?

@dnfield dnfield closed this Feb 11, 2019
@grouma
Copy link
Member

grouma commented Feb 11, 2019

I don't feel too strongly about adding it to the JSON reporter. For consistency it would be nice to have I suppose. As long as we add it in a non breaking way, see: https://github.com/dart-lang/test/blob/master/pkgs/test/doc/json_reporter.md

@dnfield
Copy link
Contributor Author

dnfield commented Feb 11, 2019

Looking at this further, I'd really want the line and column information if it's availalbe, but I don't get that in the reporter, and wiring that is becoming more work than just using the json reporter directly for my use case.

@dnfield
Copy link
Contributor Author

dnfield commented Feb 12, 2019

I've opened flutter/flutter#27845 to track this on the flutter side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants