[flutter_plugin_tools] Add a summary for successful runs

[stuartmorgan-g]

Add a summary to the end of successful runs for everything using the new looping base command, similar to what we do for summarizing failures. This will make it easy to manually check results for PRs that we know should be changing the set of run packages (adding a new package, adding a new test type to a package, adding a new test type to the tool), as well as spot-checking when we see unexpected results (e.g., looking back and why a PR didn't fail CI when we discover that it should have).

To support better surfacing skips, this restructures the return value of runForPackage to have "skip" as one of the options. As a result of it being a return value, packages that used printSkip to indicate that parts of the command were being skipped have been changed to no longer do that.

Fixes flutter/flutter#85626

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
• I updated CHANGELOG.md to add a description of the change.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
• All existing and new tests are passing.

[gaaclarke]

nit: This doesn't have to be an instance variable. It would be better off as a parameter to logSkip and logWarning. Keeping it around could lead to some weird bugs at some point because the coupling between logSkip and _currentPackage isn't obvious to the client. If you think it's cumbersome to have to pass the package to logSkip you could pass an apropos version of logSkip to the implementor of runForPackage that has the currentPackage bound.

tldr functional good, oop bad

[stuartmorgan-g]

It would be better off as a parameter to logSkip and logWarning.

I strongly considered that, since this approach feels a bit sketchy, but in practice that would be worse. With this version, the possible bugs are contained entirely within this one base class, which is short and has unit tests ensuring that these work correctly. With the parameter version, however, it's possible to write things like:

logWarning(message, example) // Oops, this was supposed to be package, not the example subdirectory.

and thus causing the warning not be summarized anywhere. That can happen in any command implementation, and is unlikely to be covered by tests.

For logSkip, the more I think about the summary, the more I think I should actually make it a return value option instead. Originally I had created printSkip for logging any case where something is being skipped, which for some commands might be, say, a subset of the examples. But with the summary we don't actually want to allow that, so I need to rework the return value. I'll land out the outstanding other conversions, then change the value to a return class with state (pass/skip/error)+error messages.

Keeping it around could lead to some weird bugs at some point because the coupling between logSkip and _currentPackage isn't obvious to the client.

For logSkip I agree. For logWarning I think that's actually a feature; a command can just logging warnings whenever it wants to as output, and doesn't have to care about what the base class might be doing with them. How they are tracked and summarized is entirely an implementation detail of the base.

[stuartmorgan-g]

This PR has been updated to eliminate logSkip by changing the return from a run to a small data class and making "skipped" a first-class result rather than a possibly-partial bit based on a log.

Commands that used to log formal skips for subsets of the command while running other subsets now just use simple print statements to indicate what is and isn't being run, and "skip" is reserved for cases where nothing is run.

[gaaclarke]

This is a huge improvement. You could eliminate _currentPackage if you created a way for PackageResult to carry the warnings too. It's not a huge deal, _currentPackage bugs me a bit since it's a looping parameter in an instance variable.

[stuartmorgan-g]

It's valid to print warnings outside the loop though; I don't want to have two different ways of handling warnings in the subclasses.

[gaaclarke]

Is hasLongOutput suppose to mean "verbose" in this case? We had another place in the code where the same verbiage meant "compress the output because it's long" I thought.

[stuartmorgan-g]

It means that in practice, the output of a given runForPackage call is likely to be long. The existing use is that it changes the header format from a single line to the ====-wrapped banner style, to make it easier to skim for a run break.

[stuartmorgan-g]

(I deliberately called it "long" rather than "verbose" because I don't want to imply that underlying commands are being called with a -v or similar flag; some commands print a lot even when you don't make them explicitly verbose.)

[gaaclarke]

Nice improvement.

[stuartmorgan-g]

Yes, the fact that I had to give the magic values [] and [''] special names in the original API should have been more of a red flag to me than it was.

[gaaclarke]

LGTM, nice update.

[gaaclarke]

nit: I don't think you need to pass in packages here, you can just loop over results.values

[stuartmorgan-g]

Results is a map, and thus doesn't have a guaranteed order; it's important that the summary be in the same order as the packages for ease of referencing up to the details.

[gaaclarke]

This is a huge improvement. You could eliminate _currentPackage if you created a way for PackageResult to carry the warnings too. It's not a huge deal, _currentPackage bugs me a bit since it's a looping parameter in an instance variable.