dart-lang / mockito

Mockito-inspired mock library for Dart
https://pub.dev/packages/mockito
Apache License 2.0
623 stars 160 forks source link

Enable MockSpec superclasses #728

Open oznecniV97 opened 6 months ago

oznecniV97 commented 6 months ago

In this PR I add a check on mockSpec type to be sure that is an effective MockSpec and not an extension of it. In this way we could use a MockSpec superclass to use default constructor parameters or added values.


Contribution guidelines:
- See our [contributor guide](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md) 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](https://dart.dev/guides/language/effective-dart) and use `dart format`. - Most changes should add an entry to the changelog and may need to [rev the pubspec package version](https://github.com/dart-lang/sdk/wiki/External-Package-Maintenance#making-a-change). - Changes to packages require [corresponding tests](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md#Testing). Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
srawlins commented 6 months ago

Thanks for the PR. Please file a bug describing what this is fixing. We also need tests showing what would fail without the changes in lib/.

oznecniV97 commented 6 months ago

Issue opened with example test: https://github.com/dart-lang/mockito/issues/729

oznecniV97 commented 5 months ago

Any update on this thread? There's some approvers?

yanok commented 4 months ago

Sorry for the delay: this project has only a single mantainer, who has more urgent tasks most of the time.

Re: discussion on the bug: It's a pity we can't have a const function, I see now, why you want to extend MockSpec.

I still don't quite like your solution though: surely, it works for your case with fallbackGenerators passed to the super constructor. But what if someone would do:

class MyMockSpec<T> extends MockSpec<T> {
  const MyMockSpec();
  @override
  final fallbackGenerators = const {#x: fallback};
}

Your code would simply ignore it.

Furthermore, it's technically possible now to implement MockSpec instead of extending it, so your code will never find MockSpec in superclasses and crash. Well, arguably that not worse than what we already have. We could also forbid this by making MockSpec base.

To cover all cases we need to call getField while going down the inheritance chain (I'm actually surprised why Analyzer can't do that for us). The only thing there you indeed want to get to MockSpec itself, is getting the type parameter, to cover weird cases like

class MyMockSpecForFoo extends MockSpec<Foo> {}
class MyMockSpecForListT<T> extends MockSpec<List<T>> {}

Would you be up for covering all of this? Especially provided that I want to do the next major release by the end of the end, hoping to be able to drop the fallback generators concept entirely. Privided there is no Analyzer API to help us here, personally I kinda feel the the code complication doesn't worth the improvement. But if you still want to follow this route, please also add cases cases covering various way to subtype MockSpec.

As a workaround you can define your fallback generators as a top-level const and pass it to every MockSpec. Not quite what you wanted to achieve, but still probably better than repeating the map every time.