CrashReportSender dependency injection

[jmagman]

Remove CrashReportSender.initializeWith in favor of dependency injection into the CrashReportSender constructor.

Remove tool.crashFileSystem in favor of the context file system.

Related Issues

#54886
#53766
#47161

Tests

Rewrote most of crash_reporting_test.

Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I signed the [CLA].
• I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
• I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• I updated/added relevant documentation (doc comments with ///).
• All existing and new tests are passing.
• The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

• No, no existing tests failed, so this is not a breaking change.
• Yes, this is a breaking change. If not, delete the remainder of this section.

[jmagman]

\cc @jamesderlin

[jmagman]

I know the double override is kind of weird, but I couldn't think of another way to do this so:

1. With a real crash, it always uses LocalFileSystem regardless of context (see previous note about recording):

/// File system used by the crash reporting logic.	
///	
/// We do not want to use the file system stored in the context because it may	
/// be recording. Additionally, in the case of a crash we do not trust the	
/// integrity of the [AppContext].

2. Still lets the test inject a MemoryFileSystem.

[jonahwilliams]

We don't support recording anymore. The latter part about not trusting AppContext is a bit suspect, I don't really think we've had any issues for the last few years regarding AppContext

[jonahwilliams]

Its a bit confusing that some of the dependencies are constructor injected and some are passed here. GitHubTemplateCreator would be more obviously optional if it were constructor injected, right?

[zanderso]

If the goal is only for internal users to see non-github error messages, then why does the whole CrashReportSender need to be moved into the context? Is there a smaller change that will solve the problem? Can the github template generator just be disabled internally, for example, and revert to the behavior before it existed?

[jamesderlin]

If the goal is only for internal users to see non-github error messages, then why does the whole CrashReportSender need to be moved into the context? Is there a smaller change that will solve the problem? Can the github template generator just be disabled internally, for example, and revert to the behavior before it existed?

In google3 we'd also like to override runner.dart's _informUserOfCrash, which currently advises people to file bugs on GitHub rather than internally. I think it would make sense for that function (and CrashReportSender) to live together. (Also see #53766.)

[zanderso]

In google3 we'd also like to override runner.dart's _informUserOfCrash, which currently advises people to file bugs on GitHub rather than internally. I think it would make sense for that function (and CrashReportSender) to live together. (Also see #53766.)

Currently, the scope of CrashReportSender is limited to sending data to crash reporting. Lumping the functionality of _informUserOfCrash into there seem like a layering violation to me. I understand the desire to customize the error messages, but this doesn't seem like the right refactoring to achieve that.

[jamesderlin]

Currently, the scope of CrashReportSender is limited to sending data to crash reporting. Lumping the functionality of _informUserOfCrash into there seem like a layering violation to me. I understand the desire to customize the error messages, but this doesn't seem like the right refactoring to achieve that.

I'd like to rename CrashReportSender to just CrashReporter. (Alternatively, there could be a separate CrashReporter class that uses a CrashReportSender, if you prefer.) Conceptually, I don't think sending crash reports and telling users about it should be divorced. And in practice, it's likely that anyone who wants to override one also would want to override the other.

[zanderso]

(Alternatively, there could be a separate CrashReporter class that uses a CrashReportSender, if you prefer.)

This sounds more promising. I expect this approach to be able to avoid the layering violation, and avoid exposing more interfaces to g3 than necessary to solve the current issue.

And in practice, it's likely that anyone who wants to override one also would want to override the other.

Both external and g3 currently send crashes to the same place. I would personally advocate against doing otherwise since it would make crash triage more difficult.

[jmagman]

I reduced the scope of this PR. It no longer moves anything into the context, it just changes the way dependencies are injected into CrashReportSender, and I re-wrote those tests to use testWithoutContext

[jonahwilliams]

LGTM!

[jonahwilliams]

(But not to CI)