fix: runZoned / withClock not working

[dkbast]

Is there an existing issue for this?

• I have searched the existing issues.

Version

0.5.1

Description

tl;dr What is the proper way to run a goldenTest in a Zone?

I want to mock DateTime.now() and for this I use the offical "clock" package.

Steps to reproduce

withClock uses runZoned under the hood and can return the callback/ result so I thought it should be possible to put it in somewhere in "goldenTest" - I tried:

builder: () => withClock<Widget>(
        Clock.fixed(DateTime(2022, 11, 17)),
        () => MyFancyWidget)

pumpWidget: (tester, widget) async => withClock(
        Clock.fixed(DateTime(2022, 11, 17)),
        () async {
          await tester.pumpWidget(widget);
        },
      ),

both did not work and the current dateTime was used instead, suggesting that for some reason the Zone was not used.

Expected behavior

I would expect both of the above examples to be valid.

This is an example of how withClock is used in a basic unit test:

test('returns a proper greeting on a Tuesday', () {
    final greeting = withClock(
      Clock.fixed(DateTime(2020, 09, 01)),
      () => Greeter().greet(),
    );

    expect(greeting, 'Happy Tuesday, you!');
# example from: https://iiro.dev/controlling-time-with-package-clock/    

Screenshots

No response

Additional context and comments

So this all turns out to be a question of "What is the proper way to run a goldenTest in a Zone?"

[dkbast]

Looks like this is a problem with tests in general:
dart-lang/test#1023
It could be fixed locally by allowing to wrap the whole test body in runZoned using a "surround" method for alchemist as proposed here dart-lang/test#377

[jolexxa]

Tagging @jeroen-meijer for his expertise here.

[inf0rmatix]

Push <3

[inf0rmatix]

my current solution is having an optional parameter in the widget for clock annotated with @visibleForTesting

class MyFancyWidget{

  const MyFancyWidget({
    @visibleForTesting Clock clock = const Clock(),
  });

  @override
  Widget build(BuildContext context) {
     final now = clock.now();
     
     return Text(now.toString());
  }
}

[jeroen-meijer]

Hi all, sorry for getting to this late, and thanks for filing the issue.

As @dkbast pointed out, this is a larger issue with the way test works. It runs in its own zone, and won't propagate user-defined values. We've seen this discrepancy before with other testing packages internally and in OSS projects.

Why this happens in Alchemist

The reason this issue arises in Alchemist is because the builder, pumpWidget and virtually all other arguments are passed to executed inside the testWidgets function internally, meaning all zone information is already lost at that point.

How Alchemist deals with this

In the past, we've solved this by saving a local copy of whatever is in the current zone to a variable and passing it to the body inside the test function. We do this with AlchemistConfig, for example, like so (truncated):

// file: lib/src/golden_test.dart

// ...

Future<void> goldenTest(
  String description, {
  // ...
}) async {
  // ...

  // Saves a local copy of the current zone's AlchemistConfig.
  // This is stored in an AlchemistTestVariant, which is eventually passed to the test,
  // circumventing the zoning issue that would otherwise make the current
  // AlchemistConfig unreachable.
  final config = AlchemistConfig.current();

  final currentPlatform = HostPlatform.current();
  final variant = AlchemistTestVariant(
    config: config,
    currentPlatform: currentPlatform,
  );

  // ...
  
  await goldenTestAdapter.testWidgets(
    description,
    (tester) async {
      final variantConfig = variant.currentConfig;
      // ...
    },
    tags: tags,
    variant: variant,
  );
}

Temporary solutions

There are multiple approaches to circumventing this issue, such as the one @inf0rmatix kindly suggested. Providing test-only values is a perfectly valid DI strategy IMO.

However, I can understand if it's not the most ergonomically attractive approach, since the monotony of passing things along like this through constructors everywhere is exactly why some of us love using Provider to pass things along through the context (an extremely similar mechanic to passing values through a Zone).

Other than that, I don't see a lot of consistent and relatively clean solutions for the time being. I would love it if someone could prove me wrong though! 😄

Long-term solution

After doing some experimenting, I have found some interesting potential solutions I would love for y'all to take a look at.
A branch with these experimental changes can be found at feat/zone-preservation (see diff).

It involves using the same mechanism as we use for the AlchemistConfig; saving a copy of the current zone in the goldenTest function and wrapping any function calls in this zone (using Zone.run).

There are two parts to this solution.

Accessing zoned values in the builder

This is relatively straightforward. There is virtually no need for widgets constructed inside the builder to know about the test zone (and, indeed, one could argue it shouldn't know it's inside a test), and these widgets have no dependencies on the test zone. Therefore, it's easy to save a copy of the current zone and run the builder callback inside it.

This is done using the following snippet. (Truncated)

// file: lib/src/golden_test.dart

@isTest
Future<void> goldenTest(
  // ...
) async {
  // ...

  final outerZone = Zone.current;

  await goldenTestAdapter.testWidgets(
    description,
    (tester) async {
      T wrapZone<T>(T Function() fn) =>
          config.runInOuterZone ? outerZone.run(fn) : fn();

      // ...

      await goldenTestRunner.run(
        // ...
        widget: wrapZone(builder),
        // ...
      );
    },
    // ...
  );
}

Creating a simple golden test that provides a zoned value above the test group and attempts to retrieve it inside the builder shows this approach works.

const zoneValueKey = #testZoneValueKey;
const providedValue = 42;

int? getZoneValue() => Zone.current[zoneValueKey] as int?;

void main() {
  runZoned(
    () {
      group('Zoned value test group', () {
        goldenTest(
          'zoned value test',
          fileName: 'zoned_value',
          textScaleFactor: 4,
          builder: () {
            final retrievedValue = getZoneValue();
            final isCorrect = retrievedValue == providedValue;

            if (!isCorrect) {
              print(
                'Expected zone value to be $providedValue, but '
                'was $retrievedValue instead. '
                'Seems like the zone value was not passed to the golden test '
                'properly.',
              );
            }

            return Text(
              '${isCorrect ? 'Correct' : 'Incorrect'}: $retrievedValue',
            );
          },
        );
      });
    },
    zoneValues: {zoneValueKey: providedValue},
  );
}

Output

AlchemistConfig integration

I added an option in AlchemistConfig called runInOuterZone to make sure people who do use the test zone in their builders (for some reason) can opt out of this change.

Accessing zoned values inside pumps and interaction

Enabling the same mechanic as above for pump actions and interaction callbacks is significantly trickier. The most predominant reason for this is that tester.pump(...) and friends don't work if they aren't run inside the Zone that was created by the test.

Wrapping all the methods in the same wrapZone(...) function is easy enough, but that means we can't call WidgetTester methods at all anymore, kinda defeating the whole point. 😋

So, at the moment, the only solutions I see to this problem that one could argue could be valuable to add to the package are:

1. A custom AlchemistWidgetTester that will inherit all method calls from the default WidgetTester, but is given the test zone instance first, so that it can execute all of its relevant methods inside that zone. I'm not a huge fan of this approach is it seems finnicky and would require upkeep.

2. BREAKING: Add an argument to all pump and interaction callbacks that will run a given function inside the outer zone. While this would work fine (this is what I've done in the above branch), changing any of the arguments of the pump and interaction callbacks would be a breaking change.

The following shows this approach working

// ...
pumpWidget: (tester, widget, runInOuterZone) {
  runInOuterZone(() {
    print('Zone value in pumpWidget: ${getZoneValue()}');
  });
  return onlyPumpWidget(tester, widget, runInOuterZone);
},
pumpBeforeTest: (tester, runInOuterZone) {
  runInOuterZone(() {
    print('Zone value in pumpBeforeTest: ${getZoneValue()}');
  });
  return onlyPumpAndSettle(tester, runInOuterZone);
},
whilePerforming: (tester, runInOuterZone) async {
  runInOuterZone(() {
    print('Zone value in whilePerforming: ${getZoneValue()}');
  });
},
// ...

Output

Like mentioned before, it does work but the breaking change in the arguments and the fact that tests will fail silently if users abuse the runInOuterZone function to run WidgetTester calls make it a hard sell for me. It also means we need to educate users about the use of this function.

There are alternatives that make the API more ergonomic or would prevent this from being a breaking change to begin with, but as far as I can think, every mitigation of any problem brings with it more complexity and maintenance in the internals of the package.

Feedback please!

If you ask me, we should leave the pump and interaction calls for what they are, but I think the zoning fix for the builder arg is an easy and welcome change. I say let's add some tests and do a release on that soon.

I'll leave it up to y'all to decide what about this you like and what you dislike.
Let me know if you have any questions or comments.

Thank you for reading.

[jeroen-meijer]

Hi @dkbast @inf0rmatix, was wondering if you had the time to look at the above comment to see if it solves your issues. :)

[dkbast]

Hi @jeroen-meijer - thank your for diving into this! From my perspective passing optional parameters is currently the best thing we can get. The experiment you did shows, that it would be possible to have zones/ overrides, but somebody accidentally breaking the test is to much of a risk for me.

[inf0rmatix]

Thanks for the effort, agree with both of you :)

[jxstxn1]

Another fix for could also be by setting a custom pumpWidget and whilePerforming function where you wrap the tester.pumpApp and tester.pumpAndSettle with the withClock Function.

Test Code

whilePerforming: (WidgetTester tester) async {
    await withClock(
        Clock.fixed(DateTime(2022)),
        () async => tester.pumpAndSettle(),
    );
    return Future.value();
},
pumpWidget: (tester, widget) async {
    await withClock(
        Clock.fixed(DateTime(2022)),
        () async => tester.pumpWidget(widget),
    );
},

I created an example repo with an fully working example: https://github.com/jxstxn1/alchemist_zone_test/