explicitly catch ArgumentError in exitsHappy(), and add tests

[christopherfujino]

Description

After #52021, exitsHappy() and exitsHappySync() only catch Exceptions but not ArgumentErrors, such as when the command being invoked doesn't exist. This PR explicitly catches ArgumentError.

Related Issues

Fixes #52702

Tests

I added tests for both exitsHappy() and exitsHappySync() that it catches both Exceptions and ArgumentErrors.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• 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

Did any tests fail when you ran them? Please read [Handling breaking changes].

• 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.
  • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
  • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
  • I wrote a migration guide: Replace with a link to your migration guide

[zanderso]

thanks!

[dnfield]

Why are we even calling it in this case?

[christopherfujino]

Sometimes we call exitsHappy to verify if the binary exists, e.g.

flutter/packages/flutter_tools/lib/src/ios/mac.dart

Line 38 in 071d4eb

bool get isInstalled {

[dnfield]

Ahhh ok, I see now, and we're catching ArgumentError from _processManager. Is this more reliable than just calling https://api.flutter.dev/flutter/package-process_process/ProcessManager/canRun.html?

[christopherfujino]

Not more reliable, but I think also not less reliable(?) The goal with processUtils was to have a small, concise API surface.

[dnfield]

Yes, I mean we could call that in here first and return false if it's false, rather than trying to catch an argument exception. That way if some other code starts throwing that we don't end up swallowing the unrelated exception.

[dnfield]

i.e. on line 541 of this file (or maybe line 539), if (!_processManager.canRun(cli.first), environment: environment)) { return false; } - probably also with either asserts or guards against cli being null or empty.

[christopherfujino]

Oh, I see what you mean

[christopherfujino]

That would be changing what this function does slightly, however, by discarding the arguments.

[zanderso]

DeviceManager.canListAnything should probably check discoverer.supportsPlatform before querying discoverer.canListAnything. The similar code in EmulatorManager in emulator.dart probably needs a similar fix. (Sorry for not suggesting this earlier, but the fix in this PR was needed as well.)

[dnfield]

RE discarding arguments - after checking canRun, you can still use the existing logic, but avoid trying to catch an argument error.

My concern with catching argument error is that it can be thrown for a number of reasons, including errors in the upstream code (e.g. someone accidentally makes the string null, or passes in an empty array).

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.