Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable MockSpec superclasses #728

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

oznecniV97
Copy link

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.


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

@srawlins
Copy link
Member

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
Copy link
Author

Issue opened with example test: #729

@oznecniV97
Copy link
Author

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

@yanok
Copy link
Contributor

yanok commented Feb 28, 2024

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.

@@ -1,3 +1,7 @@
## 5.4.5-wip

* Allow usage of GetMock superclasses.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Allow usage of GetMock superclasses.
* Allow usage of MockSpec subclasses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants