Final classes vs mocking

[slaci]

Hello,

When writing tests which uses Filedownloader I always want to use mocks instead of the real class, as I don't want to download anything. But now on 7.0+ you marked many classes with final and mockito cannot generate mocks from them anymore sadly:

Error: The class 'FileDownloader' can't be implemented outside of its library because it's a final class.
class MockFileDownloader extends _i1.Mock implements _i14.FileDownloader

Are you sure those finals are really required?

Build runner complaining for these currently:

• Database
• TaskStatusUpdate
• Batch

I created an issue for mockito about this: dart-lang/mockito#635

[781flyingdutchman]

Interesting - I had not thought about this and have never seen it mentioned in any of the design discussions about class modifiers. Interestingly they are positioned very much too help ensure consistent behavior for packages! I'm still learning about the benefits, so let me think a bit about whether and where these modifiers are really needed.

[slaci]

Sadly the only way would be to provide test/stub classes beside the real ones. In lib, because the test folder is not referencable from outside as far as I remember. But that would be very hard, or it may introduce a new dependency like mocktail. Or distribute a generated mockito stub... all sounds weird and hard to maintain.

[781flyingdutchman]

Perhaps the easiest is to not make the FileDownloader class final. The final modifier is more important for classes that are inputs in the FileDownloader class (such as Task). When you mock FileDownloader, does that in itself trigger the warnings for Database, TaskStatusUpdate and Batch, or do those only appear when you mock those classes? And why would you want to mock classes like Batch and TaskStatusUpdate?

[slaci]

No they are automatically mocked. I just mock the FileDownloader class. Maybe it is possible to prevent the mocking of those, but haven't looked into that yet.

[slaci]

Just to be exact, I'm doing this with mockito:

@GenerateNiceMocks([
  MockSpec<FileDownloader>(),
])

And this triggers the errors for Batch and the others. This is because mockito creates a SmartFake for all methods to return something by default. In 6.x eg:

class _FakeBatch_27 extends _i1.SmartFake implements _i15.Batch {
  _FakeBatch_27(
    Object parent,
    Invocation parentInvocation,
  ) : super(
          parent,
          parentInvocation,
        );
}

Then for downloadBatch it mocks the return value:

returnValue: _i5.Future<_i15.Batch>.value(_FakeBatch_27(
  ...

This is the default behavior. I think it is possible to define custom default returns with fallbackGenerators, but thats a bit tedious.

[781flyingdutchman]

Understand. That's going to be a problem though because I really don't want to open up all those classes - it's kinda the point of the class modifiers, especially for packages, as it makes future changes in the implementation much easier.
What if I created a final class FileDownloaderStub that returns sensible default values, for use in testing? It would be an extension of the base class with all methods stubbed out. I could even add setters that allow you to set the default return values, eg true or false, or a specific TaskStatus. It would not be mockable (for the same reasons) but it would allow you to inject a stub and even do some tests based on that stub's behavior.
Let me know what you think.

[slaci]

Sadly thats the only option if you want to keep the final modifiers. The problem is that every return value should be configurable, because in tests I would test what happens if enqueue returns false or true. In mockito it is possible to say when(downloader.enqueue(exactTaskInstance or any)).thenAnswer((_) async => false); or to verify that a method was called or not on the mock with verify or verifyNever. Currently I use all of these, so it would be a bit loss, but I could live with it I guess. Implementing these or some of these features manually for a stub would be a big burden I think.

[781flyingdutchman]

Yes, if you use it for testing then a stub isn't going to be good enough.

The 'proper' alternative I think is for me to define the FileDownloader interface as a separate interface class, say FileDownloaderAPI. The actual FileDownloader is marked final and implements this API. You (or I, as part of the package) then create an open class FileDownloaderStub that implements the API and produces default values. If you need a proper mock, you can then mock the stub using Mockito and do whatever you would otherwise do. All other classes are unaffected.

It requires two things:

1. Where you inject the FileDownloader class you now need to inject the FileDownloaderAPI class (i.e. you need to change the type of the variable you inject)
2. You may still need to fix the issue with fallbackGenerators. I have never used those but understand they are a simple Map<Symbol, Function> where I imagine the symbol is the method you want to provide a default value for (eg #download) and the function yields that default value, eg () => Future.value(TaskStatus.complete).

On naming, which do you think is better: FileDownloadeAPI or FileDownloaderInterface or something else?

Let me know what you think. I'm not able to do actual work on this for another ~2 weeks, but if you have time you could experiment with a fork (and I'd be happy to merge it in if it works).

[781flyingdutchman]

Sorry, one more thought (I'm learning about these new modifiers myself!): It looks like Mockito implements a class interface, not extend it. So can you check if mocking works if you change the modifier of FileDownloader to 'interface'? The main thing I'm trying to avoid is someone extending FileDownloader as that is likely to lead to problems down the road. Implementing requires a new implementation (like a mock) and is not an issue.

This would accomplish the same as the above without the need to introduce a separate interface class (API) or a change to the type of the variable you inject, so may solve all problems.

[781flyingdutchman]

HI, I found a few minutes and changed the final modifier to interface for FileDownloader and Database (they can still be instantiated and it actually may make it possible at some point to change the implementation of the database), and removed it altogether for Batch, TaskStatusUpdate and TaskProgressUpdate as those are outputs of the FileDownloader and I therefore don't care much about how they might be extended.

I got mock working using a simple @GenerateNiceMocks([MockSpec<FileDownloader>()]) on the mock branch of the repo, so please take a look and let me know if it works in your context.

[slaci]

I had time now to try it out and all of my tests are green with it, thanks.

Nice solution, when the class is marked with interface then it cannot be extended, so you also achieve the main goal.

[781flyingdutchman]

Implemented in V7.0.2

[slaci]

I had a discussion about this topic on flutter discord and the conclusion was that the recommended approach is to wrap plugin classes with selfmade wrapper classes on app side and mock those: https://docs.flutter.dev/packages-and-plugins/plugins-in-tests

So my approach was fragile, to just use MockSpec on the plugin class. So I think you could make the classes final again if you want, just make sure to handle it as a breaking change.

[781flyingdutchman]

Interesting, thanks for sharing that link. It's quite a big ask to expect developers to wrap every plugin though - I understand the benefit (and in a way I've done that within the plugin with the Database class which wraps the Localstore package) but I also believe one should be able to just use a plugin like background_downloader directly. Allowing developers to mock the class by making the interface available (as I've done now, in response to this issue) therefore feels appropriate to me. Package developers should mostly care about avoiding extension of classes that are inputs to their plugin (like Task) and care less about classes that are outputs (like TaskStatusUpdate), because if a developer implements those it doesn't affect the plugin operation.

A non-breaking plugin API change then may indeed require rebuilding the Mock, but nothing else, so I'm ok with that (and won't consider interface changes a breaking change if a normal application doesn't break - for example if I add a method to FileDownloader that a developer can choose to use or ignore). One could argue that any interface change is breaking because a developer implementing it will need to implement those changes, but that was true with Dart 2 and never considered a reason for a major version change, so I'm sticking with that policy 😃

[slaci]

fyi a discussion has risen from the above mentioned discord thread: dart-lang/language#3106