[native_assets_cli] Make package:test dev dep

[devoncarew]

• refactor the package:native_assets_cli dep on package:test; make the dep a dev_dependency instead of a regular dependency
• rename testBuildHook to validateBuildHook and testCodeBuildHook to `validateCodeBuildHook
• update the changelog

When doing an investigation into the deps of the dart cli tool, I found that this was bringing package:test all all its dependencies into the transitive closure of used packages. This refactors the package to still provide testing facilities but to no longer use package:test as a regular dep.

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

• See our contributor guide for general expectations for PRs.
• Larger or significant changes should be discussed in an issue before creating a PR.
• Contributions to our repos should follow the Dart style guide and use dart format.
• Most changes should add an entry to the changelog and may need to rev the pubspec package version.
• Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

[coveralls]

coverage: 88.679% (-0.02%) from 88.699%
when pulling e4f94f3 on refactor_native_assets_cli
into 00aae9e on main.

[dcharkes]

Okay, so it's an antipattern to wrap package:test. We'd better add that to the documentation of package:test.

Is it also an antipattern to wrap package:matcher? It would feel natural to define matchers for our own classes.

I guess getting rid of the package:test dep is a good thing, It causes issues:

• Make native asset tests more robust against flutter upgrading its pinned dependencies flutter/flutter#159715

cc @mosuem

[dcharkes]

Ditto, Validate is a term we use for something else. Also, it's an exception so maybe TestException?

[devoncarew]

I updated to VerificationException.

[devoncarew]

Okay, so it's an antipattern to wrap package:test. We'd better add that to the documentation of package:test.

Is it also an antipattern to wrap package:matcher? It would feel natural to define matchers for our own classes.

Yes, mostly. matcher brings in many fewer transitive deps, so in practice its less of an issue (but still something to be avoided). It's fine if the purpose of your package is to ship matchers, or to build functionality on top of test, but mixing that w/ a package that's used for other things - dep'd into people's regular deps - causes issues.

I guess getting rid of the package:test dep is a good thing, It causes issues:

• Make native asset tests more robust against flutter upgrading its pinned dependencies flutter/flutter#159715

I'll update the PR as per the review feedback.

[dcharkes]

Okay, so it's an antipattern to wrap package:test. We'd better add that to the documentation of package:test.
Is it also an antipattern to wrap package:matcher? It would feel natural to define matchers for our own classes.

Yes, mostly. matcher brings in many fewer transitive deps, so in practice its less of an issue (but still something to be avoided). It's fine if the purpose of your package is to ship matchers, or to build functionality on top of test, but mixing that w/ a package that's used for other things - dep'd into people's regular deps - causes issues.

Ah right. We would make a separate package:native_toolchain_c_test then. We could consider that once we can change this repo to a pub workspace (#1223), before that it's too painful to have many packages.

[devoncarew]

Updated!

[devoncarew]

I updated to VerificationException.

[dcharkes]

One more comment 🤓

[dcharkes]

I don't think this class should be part of the public API. I don't believe users should catch it. So maybe it should an Error instead? Error itself has a message, so we could even throw Error instead of _VerificationError.

Or do you believe users should catch this?

[devoncarew]

OK, I looked into this a bit more. Error itself doesn't have a message; its subclasses do. None of those look like great candidates to throw, and, it looks like expect throws an Exception, not an Error, so I think we should do the same.

I updated this to be a ValidationFailure, implementing an Exception and not an error. I also moved it to a place in the code where it won't be public API.

[dcharkes]

lgtm! Please add auto-submit at your convenience (after rebase).

Thanks @devoncarew! 🙏