Skip to content

@GenerateMocks does not work on Generic method return types #338

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

Closed
MaxAuer opened this issue Feb 23, 2021 · 27 comments
Closed

@GenerateMocks does not work on Generic method return types #338

MaxAuer opened this issue Feb 23, 2021 · 27 comments

Comments

@MaxAuer
Copy link

MaxAuer commented Feb 23, 2021

Hey,

I tried to create a Mock for the following Function:

T add<T extends Base>(T data);

Several Classes are extending Base.

flutter pub run build_runner build throws the following error The method 'Api.add' features a non-nullable unknown return type, and cannot be stubbed.

I am using Mockito: ^5.0.0-nullsafety.7

Any way to get around this?

@srawlins srawlins changed the title @GenerateMocks does not work on Generics on NullSafety @GenerateMocks does not work on Generic method return types Feb 23, 2021
@srawlins
Copy link
Member

Good question. It will be tricky, and I think we'll need to add new functionality. There are really two problems here. Looking at an example class:

class C {
  int get m => 0;
  String s(String a) => a;
  /// 100 other methods you'd like to stub...
  T add<T>(T data) => data;
}
  1. The broad, more common problem is that mockito right now will refuse to generate a stub for this entire class, because "The method 'C.add' features a non-nullable unknown return type, and cannot be stubbed." There is no mechanism to write code that instantiates T values at compile time. (Even for this case where you are offered a free parameter of type T, mockito always expands parameter types to be nullable (T?), so the parameter is not usable as a dummy return value. A user might say, "I don't care about add, I just need to stub the other 100 methods." I think this issue is fairly high priority.
  2. The more acute, and less common problem is that a user says, "I'd like to stub add. How do I do it?"
    CC @TedSander @matanlurey @natebosch for input.

We can solve the problems separately. There are a few ways to solve the first:

  1. Instead of throwing this error at compile-time, implement an add function which just throws at runtime. This could be implemented as opt-in or opt-out behavior when declaring a custom mock.

  2. We could allow users to declare a mixin of their own devising which is mixed into the generated class. Something like:

    mixin MockCAddMixin on C {
      T add<T>(T data) => data; // dummy function satisfying implementation
    }

    and the generated code looks something like:

    class MockC extends Mock implements C with MockCAddMixin {
      // No implementations of any methods found in MockCAddMixin.
      // ...
    }
  3. For any methods like add, we always add an optional named or positional parameter, something like T dummyReturnValue, which must be supplied (but must be positional for type system reasons). This is very distasteful for a few reasons. If add has optional positional parameters, it would have to be positional, and come last, and so users would always have to pass arguments for every parameter. Yuck. If add has optional named parameters, then the new parameter would have to be named, which is now a weird inconsistency. This also exposes ugly internals of mockito, requiring users to create a T, and pass it, and it is not at all part of the stubbing, it's just a necessary evil object.

I haven't really thought of solutions for the second problem. Maybe user-authored mixin would work here too, if it can somehow call Mock.noSuchMethod...

@MaxAuer
Copy link
Author

MaxAuer commented Feb 23, 2021

Thanks for the detailed response @srawlins.

I agree that problem 1. is the more common and probably the issue that will surface a lot when null safety becomes the new default.

Solution 2. of using a Mixin to fall back to sounds good enough to handle such cases. I also agree that 3. does not seem to be user-friendly.

The second problem is definitely the one I am stuck on right now. But more so in combination with Futures #339. I think those two problems are pretty similar.

@srawlins
Copy link
Member

Regarding the second problem, I think you can even get real mock support. Here is a rough mockup showing how you can use super.noSuchMethod to route back into mockito's stubbing and verification mechanics:

class Mock {
  noSuchMethod(Invocation i) {
    print(i.positionalArguments);
    print('mock.nsm');
    return 6;
  }
}

class C {
  T f<T>(T obj) => obj;
}

mixin Mixin on Mock, C {
  T f<T>(T obj) {
    if (obj is int) {
      return super.noSuchMethod(Invocation.genericMethod(#f, [int], [1]));
    } else if (obj is double) {
      return super.noSuchMethod(Invocation.genericMethod(#f, [double], [1.5]));
    }
    throw 'boo';
  }
}

abstract class _MockC extends Mock implements C {}

class MockC extends _MockC with Mixin {}

void main() {
  MockC c = MockC();
  c.f(7);
  c.f(7.5);
}

@natebosch
Copy link
Member

Would it be feasible to allow defining a top-level method with a matching generic signature, and configure the mock to defer to that implementation when there is no stub?

@srawlins
Copy link
Member

srawlins commented May 7, 2021

@natebosch very cool idea. I'm going to prototype this. I haven't been very happy with my mixin idea because of the generics issues.

@eggnstone
Copy link

Do you have a workaround for problem 1?
I looked at #342 but it's not clear to me if this is a working solution. E.g. "mixingIn" is not a param for MockSpec.

@srawlins
Copy link
Member

Right its not a solution yet. I prototyped the top-level method idea this weekend and hope to loop back with my results today.

@eggnstone
Copy link

Any news or ETA please?

@srawlins
Copy link
Member

This will likely land within the next 40 days.

@srawlins
Copy link
Member

This is available now with Mockito 5.0.9. Please see the new section in the NULL_SAFETY_README:

https://github.com/dart-lang/mockito/blob/master/NULL_SAFETY_README.md#fallback-generators

@ngxingyu
Copy link

Works well for me! thanks

@eggnstone
Copy link

I read the readme and it suggests to add an mShim method.
But what if I need mocks for classes outside of my control e.g. DocumentSnapshot from the cloud_firestore package?

@eggnstone
Copy link

I updated to 5.0.9 and tried the following

DocumentReference<R> withConverter<R>({required FromFirestore<R> fromFirestore, required ToFirestore<R> toFirestore})
=> throw Exception();

@GenerateMocks([], customMocks: [MockSpec<DocumentReference>(as: #MockDocumentReference, {#withConverter: mockWithConverter})])

but got the error
error: Too many positional arguments: 0 expected, but 1 found. (extra_positional_arguments_could_be_named at xyz
with the cursor then positioned at the beginning of {#withConverter:

@eggnstone
Copy link

Solution:
the fallbackGenerators is a named parameter! The doc is not up-to-date.
Correct code would be:
@GenerateMocks([], customMocks: [MockSpec<DocumentReference>(as: #MockDocumentReference, fallbackGenerators: {#withConverter: mockWithConverter})])

@eggnstone
Copy link

Compilation of created mock file fails:

Source code:

import 'package:cloud_firestore/cloud_firestore.dart';
import 'package:mockito/annotations.dart';
import 'package:test/test.dart';

import 'FirebaseFirestoreX2_test.mocks.dart';

DocumentReference<R> withConverterForDocumentReference<R>({required FromFirestore<R> fromFirestore, required ToFirestore<R> toFirestore})
=> throw Exception();

@GenerateMocks(<Type>[], customMocks: <MockSpec<dynamic>>[
    MockSpec<DocumentReference>(as: #MockDocumentReference, fallbackGenerators: {#withConverter: withConverterForDocumentReference})
])
void main()
{
    test('DocumentReference test', ()
    async
    {
        MockDocumentReference<Map<String, dynamic>> mock = MockDocumentReference<Map<String, dynamic>>();
    });
}

Created mock file:

// Mocks generated by Mockito 5.0.9 from annotations
// in xyz/tests/core/infrastructure/core/FirebaseFirestoreX2_test.dart.
// Do not manually edit this file.

import 'dart:async' as _i3;

import 'package:cloud_firestore/cloud_firestore.dart' as _i2;
import 'package:cloud_firestore_platform_interface/src/get_options.dart' as _i4;
import 'package:cloud_firestore_platform_interface/src/set_options.dart' as _i5;
import 'package:mockito/mockito.dart' as _i1;

import 'FirebaseFirestoreX2_test.dart' as _i6;

// ignore_for_file: avoid_redundant_argument_values
// ignore_for_file: comment_references
// ignore_for_file: invalid_use_of_visible_for_testing_member
// ignore_for_file: prefer_const_constructors
// ignore_for_file: unnecessary_parenthesis

class _FakeFirebaseFirestore extends _i1.Fake implements _i2.FirebaseFirestore {
}

class _FakeCollectionReference<T extends Object?> extends _i1.Fake
    implements _i2.CollectionReference<T> {}

class _FakeDocumentSnapshot<T extends Object?> extends _i1.Fake
    implements _i2.DocumentSnapshot<T> {}

/// A class which mocks [DocumentReference].
///
/// See the documentation for Mockito's code generation for more information.
// ignore: must_be_immutable
class MockDocumentReference<T extends Object?> extends _i1.Mock
    implements _i2.DocumentReference<T> {
  MockDocumentReference() {
    _i1.throwOnMissingStub(this);
  }

  @override
  _i2.FirebaseFirestore get firestore =>
      (super.noSuchMethod(Invocation.getter(#firestore),
          returnValue: _FakeFirebaseFirestore()) as _i2.FirebaseFirestore);
  @override
  String get id =>
      (super.noSuchMethod(Invocation.getter(#id), returnValue: '') as String);
  @override
  _i2.CollectionReference<Object?> get parent =>
      (super.noSuchMethod(Invocation.getter(#parent),
              returnValue: _FakeCollectionReference<Object?>())
          as _i2.CollectionReference<Object?>);
  @override
  String get path =>
      (super.noSuchMethod(Invocation.getter(#path), returnValue: '') as String);
  @override
  _i2.CollectionReference<Map<String, dynamic>> collection(
          String? collectionPath) =>
      (super.noSuchMethod(Invocation.method(#collection, [collectionPath]),
              returnValue: _FakeCollectionReference<Map<String, dynamic>>())
          as _i2.CollectionReference<Map<String, dynamic>>);
  @override
  _i3.Future<void> delete() =>
      (super.noSuchMethod(Invocation.method(#delete, []),
          returnValue: Future<void>.value(),
          returnValueForMissingStub: Future.value()) as _i3.Future<void>);
  @override
  _i3.Future<void> update(Map<String, Object?>? data) =>
      (super.noSuchMethod(Invocation.method(#update, [data]),
          returnValue: Future<void>.value(),
          returnValueForMissingStub: Future.value()) as _i3.Future<void>);
  @override
  _i3.Future<_i2.DocumentSnapshot<Object?>> get([_i4.GetOptions? options]) =>
      (super.noSuchMethod(Invocation.method(#get, [options]),
              returnValue: Future<_i2.DocumentSnapshot<Object?>>.value(
                  _FakeDocumentSnapshot<Object?>()))
          as _i3.Future<_i2.DocumentSnapshot<Object?>>);
  @override
  _i3.Stream<_i2.DocumentSnapshot<Object?>> snapshots(
          {bool? includeMetadataChanges = false}) =>
      (super.noSuchMethod(
              Invocation.method(#snapshots, [],
                  {#includeMetadataChanges: includeMetadataChanges}),
              returnValue: Stream<_i2.DocumentSnapshot<Object?>>.empty())
          as _i3.Stream<_i2.DocumentSnapshot<Object?>>);
  @override
  _i3.Future<void> set(Object? data, [_i5.SetOptions? options]) =>
      (super.noSuchMethod(Invocation.method(#set, [data, options]),
          returnValue: Future<void>.value(),
          returnValueForMissingStub: Future.value()) as _i3.Future<void>);
  @override
  _i2.DocumentReference<R> withConverter<R>(
          {_i2.FromFirestore<R>? fromFirestore,
          _i2.ToFirestore<R>? toFirestore}) =>
      (super.noSuchMethod(
          Invocation.method(#withConverter, [],
              {#fromFirestore: fromFirestore, #toFirestore: toFirestore}),
          returnValue: _i6.withConverterForDocumentReference<R>(
              fromFirestore: fromFirestore,
              toFirestore: toFirestore)) as _i2.DocumentReference<R>);
}

Errors:

  • error: 'MockDocumentReference.parent' ('CollectionReference<Object?> Function()') isn't a valid override of 'DocumentReference.parent' ('CollectionReference<T> Function()'). (invalid_override at [xyz] tests\core\infrastructure\core\FirebaseFirestoreX2_test.mocks.dart:47)
  • error: Missing type arguments for generic type 'Future<dynamic>'. (implicit_dynamic_type at [xyz] tests\core\infrastructure\core\FirebaseFirestoreX2_test.mocks.dart:64)
  • error: Missing type arguments for generic type 'Future<dynamic>'. (implicit_dynamic_type at [xyz] tests\core\infrastructure\core\FirebaseFirestoreX2_test.mocks.dart:69)
  • error: 'MockDocumentReference.get' ('Future<DocumentSnapshot<Object?>> Function([GetOptions?])') isn't a valid override of 'DocumentReference.get' ('Future<DocumentSnapshot<T>> Function([GetOptions?])'). (invalid_override at [xyz] tests\core\infrastructure\core\FirebaseFirestoreX2_test.mocks.dart:71)
  • error: 'MockDocumentReference.snapshots' ('Stream<DocumentSnapshot<Object?>> Function({bool? includeMetadataChanges})') isn't a valid override of 'DocumentReference.snapshots' ('Stream<DocumentSnapshot<T>> Function({bool includeMetadataChanges})'). (invalid_override at [xyz] tests\core\infrastructure\core\FirebaseFirestoreX2_test.mocks.dart:77)
  • error: Missing type arguments for generic type 'Future<dynamic>'. (implicit_dynamic_type at [xyz] tests\core\infrastructure\core\FirebaseFirestoreX2_test.mocks.dart:88)
  • error: The argument type 'R Function(DocumentSnapshot<Map<String, dynamic>>, SnapshotOptions?)?' can't be assigned to the parameter type 'R Function(DocumentSnapshot<Map<String, dynamic>>, SnapshotOptions?)'. (argument_type_not_assignable at [xyz] tests\core\infrastructure\core\FirebaseFirestoreX2_test.mocks.dart:97)
  • error: The argument type 'Map<String, Object?> Function(R, SetOptions?)?' can't be assigned to the parameter type 'Map<String, Object?> Function(R, SetOptions?)'. (argument_type_not_assignable at [xyz] tests\core\infrastructure\core\FirebaseFirestoreX2_test.mocks.dart:98)

@ngxingyu
Copy link

@eggnstone I would like to share with you my working setup (I only needed to use a few of the methods, but hopefully it is useful). I believe you will have to mock the CollectionReference<Map<String, dynamic>> as well.

MockDocumentReference docShim<T>(String? a) {
  if (a is String)
    return MockDocumentReference();
  else {
    throw Exception();
  }
}
Map<String, dynamic> dataShim<T>() => {};

@GenerateMocks([], customMocks: [
  MockSpec<CollectionReference<Map<String, dynamic>>>(
      as: #MockCollectionReference, fallbackGenerators: {#doc: docShim}),
  MockSpec<FirebaseFirestore>(as: #MockFirebaseFirestore),
  MockSpec<DocumentReference<Map<String, dynamic>>>(as: #MockDocumentReference),
  MockSpec<DocumentSnapshot<Map<String, dynamic>>>(as: #MockDocumentSnapshot),
  MockSpec<Query<Map<String, dynamic>>>(as: #MockQuery),
  MockSpec<QuerySnapshot<Map<String, dynamic>>>(as: #MockQuerySnapshot),
  MockSpec<QueryDocumentSnapshot<Map<String, dynamic>>>(
      as: #MockQueryDocumentSnapshot, fallbackGenerators: {#data: dataShim})
])

@eggnstone
Copy link

Thank you @ngxingyu this is a good starting point!

I however need the withConverter<R> method as well and that's where the trouble starts:

Using

CollectionReference<R> withConverterForCollectionReference<R extends Object?>({required FromFirestore<R> fromFirestore, required ToFirestore<R> toFirestore})
=> throw Exception();

@GenerateMocks(<Type>[],
    customMocks: <MockSpec<dynamic>>[
        MockSpec<CollectionReference<Map<String, dynamic>>>(
            as: #MockCollectionReferenceMapStringDynamic,
            fallbackGenerators: <Symbol, Function>{
                #doc: docShim,
                #withConverter: withConverterForCollectionReference
            }
        )
    ]
)

I get

tests/core/infrastructure/core/FirebaseFirestoreX_test.mocks.dart:284:30: 
Error: The argument type 'R Function(DocumentSnapshot<Map<String, dynamic>>, SnapshotOptions?)?' 
can't be assigned to the parameter type 'R Function(DocumentSnapshot<Map<String, dynamic>>, SnapshotOptions?)' 
because 'R Function(DocumentSnapshot<Map<String, dynamic>>, SnapshotOptions?)?' 
is nullable and 'R Function(DocumentSnapshot<Map<String, dynamic>>, SnapshotOptions?)' isn't.

@ngxingyu
Copy link

@eggnstone Actually would something like this suffice? https://gist.github.com/ngxingyu/51b0e40422fedb4253f57babecd3d999
The only fallback generator I needed to use was for the QueryDocumentSnapshot #data method, and I could use the MockDocumentReference withConverter with generic types as well.

cloud_firestore: ^2.2.0
mockito: ^5.0.9

@eggnstone
Copy link

@ngxingyu That works, thank you!

@eggnstone
Copy link

New problems arise when the dataShim method is actually called because now it can't be prepared with when().
When calling when(abc.data()).thenReturn(...); the dataShim gets immediately called.

My first try was to build several dataShim functions in order to return what I want but that doesn't work if you need more than one different result.

@ngxingyu
Copy link

ngxingyu commented Jun 1, 2021

I didn't actually face this issue, my approach is actually to create a MockDocumentSnapshot for each usecase, so there's no need to return different responses for the same snapshot. I've updated my gist to give a slightly more detailed example.

@eggnstone
Copy link

eggnstone commented Jun 1, 2021

This works perfectly for the mocked classes without the dataShim.

If you need to

  • mock a class that requires a dataShim via the fallbackGenerators
  • and have 2 or more aliases

then it does not work anymore. This maybe a bug with the fallbackGenerators @srawlins:
Example:

MockSpec<QueryDocumentSnapshot<C>>(as: #MockQueryDocumentSnapshotC1, fallbackGenerators: {#data: dataC1}),
MockSpec<QueryDocumentSnapshot<C>>(as: #MockQueryDocumentSnapshotC2, fallbackGenerators: {#data: dataC2}),

Try calling MockQueryDocumentSnapshotC1.data() vs. MockQueryDocumentSnapshotC2.data().
They both use the same dataC2 function.
It seems like that last #data declaration overwrites all previous ones of that type.

And preparing via when() does not work for these data methods.

@srawlins
Copy link
Member

srawlins commented Jun 7, 2021

@eggnstone Yikes I can reproduce this. Not great.

@srawlins
Copy link
Member

srawlins commented Jun 7, 2021

@eggnstone I think I can only reproduce this when the two types being mocked are identical, like QueryDocumentSnapshot<C> and QueryDocumentSnapshot<C>. (By the way, what is this class, C?)

Mockito does not explicitly support generating two mocks for identical types...

@eggnstone
Copy link

@srawlins

@eggnstone I think I can only reproduce this when the two types being mocked are identical,
I only did that because I couldn't use when() on the instances.

By the way, what is this class, C?
It's a plain DTO created with freezed.

@wesleyfuchter
Copy link

Hey there! Any updates on this? I'm facing the same problem with Firestore's QueryDocumentSnapshot data method.

@srawlins
Copy link
Member

This issue is outdated and contains several threads. Mockito 5.1.0 contains an alternative to fallbackGenerators on MockSpec called unsupportedMethods.

Please open a new issue for any problem observed with Mockito >=5.1.0 which cannot be solved with fallbackGenerators or unsupportedMethods.

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

No branches or pull requests

6 participants