mockito
mockito copied to clipboard
Enable MockSpec superclasses
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:
- 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.
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/
.
Issue opened with example test: https://github.com/dart-lang/mockito/issues/729
Any update on this thread? There's some approvers?
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.