mockito icon indicating copy to clipboard operation
mockito copied to clipboard

Enable MockSpec superclasses

Open oznecniV97 opened this issue 1 year ago • 4 comments

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.


  • [x] I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

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

oznecniV97 avatar Dec 20 '23 23:12 oznecniV97

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/.

srawlins avatar Dec 21 '23 00:12 srawlins

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

oznecniV97 avatar Dec 21 '23 16:12 oznecniV97

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

oznecniV97 avatar Jan 26 '24 16:01 oznecniV97

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.

yanok avatar Feb 28 '24 16:02 yanok