felangel / mocktail

A mock library for Dart inspired by mockito
https://pub.dev/packages/mocktail
MIT License
617 stars 81 forks source link

mocktail_image_network: `mockNetworkImages` affects other tests #134

Closed jjochen closed 4 months ago

jjochen commented 2 years ago

Describe the bug

Mocking network images in one test will mock them for all upcoming tests in the same file.

To Reproduce Updated tests from the example:


void main() {
  testWidgets('can use mocktail for network images', (tester) async {
    await mockNetworkImages(() async {
      await tester.pumpWidget(FakeApp());
      expect(find.byType(Image), findsOneWidget);
    });
  });

  testWidgets('this test should fail', (tester) async {
    // This test fails if run separately without the above test (as expected),
    // but succeeds when run after the above test.

    await tester.pumpWidget(FakeApp());
  });
}

Running the second test after the first succeeds unexpectedly.

Expected behavior Mocking network images in one test should not affect other tests.

kturney commented 1 year ago

FWIW, we started experiencing a similar issue in our local fork after down streaming https://github.com/felangel/mocktail/pull/120.

Turns out the the first mock ended up in the imageCache, and so future image requests never called _createHttpClient.

We fixed it by implementing our local mockNetworkImages like so

Future<T> mockNetworkImages<T>(FutureOr<T> Function() body) async {
  final currentClientProvider = debugNetworkImageHttpClientProvider;
  try {
    debugNetworkImageHttpClientProvider = _createHttpClient;
    return await body();
  } finally {
    debugNetworkImageHttpClientProvider = currentClientProvider;
    TestWidgetsFlutterBinding.ensureInitialized().imageCache.clear();
  }
}
felangel commented 1 year ago

@jjochen sorry for the delay! Can you verify whether the following resolves the issue?

void main() {
  testWidgets('can use mocktail for network images', (tester) async {
    final app = mockNetworkImages(() => FakeApp());
    await tester.pumpWidget(app);
    expect(find.byType(Image), findsOneWidget);
  });

  testWidgets('this test should fail', (tester) async {
    await tester.pumpWidget(FakeApp());
  });
}

@kturney are you able to provide a minimal reproduction sample?

kturney commented 1 year ago

Unfortunately, I probably won't have a chance to MR a failing test anytime soon. But I can describe the failure we were seeing.

We had a test that runs with several variants. The test has an expect that checks for a loading state, expecting the image to not yet be loaded. The expect would pass for the first variant, but all subsequent variants would fail because the image was already loaded and so skipped past showing loading state.

I hypothesize we didn't encounter this prior to https://github.com/felangel/mocktail/pull/120 because the request was never added to the imageCache since we hadn't been telling the imageCache the request was done.

But now subsequent tests just immediately load the image from imageCache.

felangel commented 1 year ago

I'm wondering whether scoping the mockNetworkImage to exclusively the widget would also resolve the issues you were encountering.

BAD:

void main() {
  testWidgets('...', (tester) async {
    await mockNetworkImages(() async {
      await tester.pumpWidget(MyWidget());
      // ...
    });
  });
}

GOOD:

void main() {
  testWidgets('...', (tester) async {
    final widget = mockNetworkImages(() => MyWidget());
    await tester.pumpWidget(widget);
    // ...
  });
}
kturney commented 1 year ago

At least for our use case, we enforce only using the "BAD" form because otherwise we encountered spurious image requests that occurred outside the mock and would therefore get a 400.

We also use debugNetworkImageHttpClientProvider instead of HttpOverrides to work around https://github.com/flutter/flutter/issues/83901.

I see there was an apparently abandoned attempt to fix HttpOverides in https://github.com/flutter/flutter/pull/83952.

SimonWeidemann commented 1 year ago

@felangel Can we adapt the examples to your scoping solution and mention that in the description? So that other people are aware that incorrect scoping leads to problems? I am willing to create a pull request for this.

felangel commented 1 year ago

@felangel Can we adapt the examples to your scoping solution and mention that in the description? So that other people are aware that incorrect scoping leads to problems? I am willing to create a pull request for this.

Yes definitely! PRs are welcome otherwise I should have time later today.

jjochen commented 1 year ago

@jjochen sorry for the delay! Can you verify whether the following resolves the issue?

@felangel I tested the "GOOD" approach from above, but as far as I can tell, it doesn't mock the network image at all.

testWidgets('can use mocktail for network images', (tester) async {
    final app = mockNetworkImages(() => FakeApp());
    await tester.pumpWidget(app);
    expect(find.byType(Image), findsOneWidget);
  });

throws an NetworkImageLoadException.

felangel commented 4 months ago

@jjochen you'll need to also move the pumpWidget call into the `mockNetworkImages closure.

Since this issue is quite old, I'm going to close but if this is still an issue for folks let me know (and ideally provide a minimal reproduction sample) and I'm happy to take a closer look at what we can do to improve things, thanks!