mocktail icon indicating copy to clipboard operation
mocktail copied to clipboard

mocktail_image_network: `mockNetworkImages` affects other tests

Open jjochen opened this issue 2 years ago • 8 comments

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.

jjochen avatar Jun 01 '22 09:06 jjochen

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();
  }
}

kturney avatar May 17 '23 04:05 kturney

@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?

felangel avatar May 17 '23 04:05 felangel

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.

kturney avatar May 17 '23 04:05 kturney

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);
    // ...
  });
}

felangel avatar May 17 '23 04:05 felangel

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.

kturney avatar May 17 '23 05:05 kturney

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

SimonWeidemann avatar May 28 '23 11:05 SimonWeidemann

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

felangel avatar May 28 '23 14:05 felangel

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

jjochen avatar May 30 '23 11:05 jjochen

@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!

felangel avatar Apr 20 '24 03:04 felangel