Skip to content

Instantly share code, notes, and snippets.

Show Gist options
  • Save christopherfujino/d7356f83d3a3d945c5ef0eeec97aab35 to your computer and use it in GitHub Desktop.
Save christopherfujino/d7356f83d3a3d945c5ef0eeec97aab35 to your computer and use it in GitHub Desktop.
How the tool uses globals

How the tool uses globals

Consider the following minimal command line application that has two sub-commands ("foo" and "bar") and a single Logger instance shared between the two:

// example_00.dart
void main(List<String> args) {
  return switch (args.first) {
    'foo' => fooCommand(),
    'bar' => barCommand(),
    _ => throw StateError('Usage error'),
  };
}

void fooCommand() {
  Logger.instance.log('running "foo"');
}

void barCommand() {
  Logger.instance.log('running "bar"');
}

class Logger {
  const Logger._();

  static const Logger instance = Logger._();

  void log(String message) => print(message);
}

Note that static variables are global. You have to reference them via a namespace, which is nice, but they are still global variables. Because Logger has only a single private contructor, if we moved the class definition to its own library, we could be certain that an instance can only be obtained via our singleton Logger.instance static variable (although it could still be sub-classed).

Now what happens when we want to unit test our fooCommand() and barCommand() functions? We have no mechanism for overriding in a test what Logger is used (let's ignore the IOOverrides class in dart:io for now, as it is equivalent to an approach we will look at later).

The simplest solution would be to make Logger.instance non-const, and re-assign it before each test:

// example_01.dart
import 'package:test/test.dart';

void main() {
  late _TestLogger _testLogger;

  setUp(() {
    _testLogger = _TestLogger();
    Logger.instance = _testLogger;
  });

  test('fooCommand', () {
    fooCommand();
    expect(_testLogger.lines.first, contains('foo'));
  });

  test('barCommand', () {
    barCommand();
    expect(_testLogger.lines.first, contains('bar'));
  });
}

void fooCommand() {
  Logger.instance.log('running "foo"');
}

void barCommand() {
  Logger.instance.log('running "bar"');
}

/// Our entire app shares the [instance] singleton.
class Logger {
  const Logger._();

  static Logger instance = Logger._();

  void log(String message) => print(message);
}

class _TestLogger implements Logger {
  final List<String> lines = <String>[];

  void log(String message) => lines.add(message);
}

This is bad however because we now have global mutable state. If we later add another test library, but forget to add a setUp() invocation we will leak our _TestLogger between tests. Also, with large test files with nested group()s, it can be difficult to reason about when a setUp() callback is invoked.

It would be great if there was a way for our business logic code (in this case, fooCommand() and barCommand()) to retrieve a different Logger instance based on the context it is running in. Fortunately you can with Zones!

// example_02.dart
import 'dart:async';
import 'package:test/test.dart';

void main() {
  late _TestLogger _testLogger;

  setUp(() {
    _testLogger = _TestLogger();
  });

  test('fooCommand', () {
    runZoned(
      () => fooCommand(),
      zoneValues: {'logger': _testLogger}
    );
    expect(_testLogger.lines.first, contains('foo'));
  });

  test('barCommand', () {
    runZoned(
      () => barCommand(),
      zoneValues: {'logger': _testLogger}
    );
    expect(_testLogger.lines.first, contains('bar'));
  });
}

void fooCommand() {
  Logger.instance.log('running "foo"');
}

void barCommand() {
  Logger.instance.log('running "bar"');
}

/// [Logger.instance] now uses Zones!
class Logger {
  const Logger._();

  static Logger get instance => Zone.current['logger'] ?? const Logger._();

  void log(String message) => print(message);
}

class _TestLogger implements Logger {
  final List<String> lines = <String>[];

  void log(String message) => lines.add(message);
}

This doesn't look too bad. We've just added a few more lines of code: each test that wants to override a Zone.current access needs to be wrapped and our Logger.instance is now a getter that will first check [Zone.current] for a Logger and fall back to our singleton (here it is a singleton because we invoke the const constructor, we would need some extra typing if we needed a non-const constructor).

But what the heck is a Zone anyway?

A zone represents the asynchronous dynamic extent of a call. It is the computation that is performed as part of a call and, transitively, the asynchronous callbacks that have been registered by that code.

Hmm, that's pretty confusing. Zones also complicate debugging, as it is non-obvious from reading the code where and when a value from Zone.current was constructed.

In addition, how would a future author of a new test that calls fooCommand() know that they have to override the 'logger' field in zoneValues? Since our Logger.instance getter will silently fall back to our desired production instance, it would be easy for a new test to accidentally use the production Logger. With this particular example, hopefully the author would notice the stray prints in the test output. But what if global value was an HttpClient? Or a LocalFileSystem, and the test is actually deleting files on the developer's computer?

One solution would be to be strict about never having any fallbacks, and having any function that checks for a value in the current Zone to throw if one was not provided. This way, any production code that depended on a global zone value that is invoked by a test that had not set up that override would fail, and the test author would know they have to provide an appropriate test object. If there was a test wrapper function (e.g. testUsingContext()), should it allow default fallbacks? Fallbacks would automagically make certain tests much easier to write at the cost of having some tests pass for the wrong reason because the test author is unaware their test is silently using a fake object.

Another solution, which trades ease of writing for ease of understanding, is dependency injection:

// example_03.dart
import 'package:test/test.dart';

void main() {
  late _TestLogger _testLogger;

  setUp(() {
    _testLogger = _TestLogger();
  });

  test('fooCommand', () {
    fooCommand(_testLogger);
    expect(_testLogger.lines.first, contains('foo'));
  });

  test('barCommand', () {
    barCommand(_testLogger);
    expect(_testLogger.lines.first, contains('bar'));
  });
}

void fooCommand(Logger logger) {
  logger.log('running "foo"');
}

void barCommand(Logger logger) {
  logger.log('running "bar"');
}

class Logger {
  const Logger._();

  static const Logger instance = Logger._();

  void log(String message) => print(message);
}

class _TestLogger implements Logger {
  final List<String> lines = <String>[];

  void log(String message) => lines.add(message);
}

In this trivial example, I think this is clearly the best solution:

  1. It is easy from reading the code to reason about the lifecycle of the Logger that each sub-command uses (the construction of each Logger can easily be found in a debugger by walking up the stack trace).
  2. Any new test calling a sub-command would be required by the compiler to explicitly pass a Logger argument, and thus test authors would have a local reference they can assert on (with testUsingContext, imperative set up steps to an override can be cumbersome).
  3. All dependencies of a test are clear at a glance in code review.

The drawbacks are:

  1. A deeply nested function requiring a new reference to a global object (such as a Logger) may require updating many other function signatures to pass that object down the stack.
  2. Updating any of those function signatures may require many tests to be updated, to now pass test overrides.
  3. Complex functions (such as sub-command constructors) will now have many parameters.
@eliasyishak
Copy link

This is interesting, for your second approach with the zones, what would it look like if we had a Map of the context classes for production somewhere and it was referenced in for the production context

Map productionContext = {
  "logger": Logger(),
  "classA": ClassA(),
  ...
}

void main() {
  runZoned(
    () => barCommand(),
    zoneValues: productionContext
  );
}

But when we want to run tests where we want to override some of the classes with fakes, we can overwrite the key in the productionContext map with the class we want. For example, in your example you have _TestLogger for testing purposes so it could look like

Map productionContext = {
  "logger": Logger(),
  "classA": ClassA(),
  ...
}

test('barCommand', () {
  runZoned(
    () => barCommand(),
    zoneValues: {...productionContext, 'logger': _TestLogger()} // You would overwrite the classes needed for tests
  );
  expect(_testLogger.lines.first, contains('bar'));
});

This does have the drawback that the test writer would need to know every class they would need to overwrite but at least if they are seeing behavior they aren't expecting, it would be easy to find one location where all the classes are being provided

@christopherfujino
Copy link
Author

You mean having a mutable global that we overwrite? Isn't that basically example_01.dart?

@christopherfujino
Copy link
Author

Oh wait, nvm, looking closer at your example, I see what you mean. I mean, this is basically what we have now with testUsingContext, right?

@eliasyishak
Copy link

Ah could be, is that what this snippet is referring to https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/src/context.dart#L104-L122 ?

I didn't realize that that testUsingContext also had its own preconfigured overrides in addition to any other overrides the test writer may need

@christopherfujino
Copy link
Author

Ah could be, is that what this snippet is referring to https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/src/context.dart#L104-L122 ?

Yeah. There's also a whole other abstraction for this at https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/src/testbed.dart#L32

I didn't realize that that testUsingContext also had its own preconfigured overrides in addition to any other overrides the test writer may need

Yeah, our testing "helper" situation is incredibly complex.

@andrewkolos
Copy link

andrewkolos commented Jun 9, 2023

Thanks for the writeup. I think everything here makes sense. I do like manual DI in that it declares a dependency of a class in a highly-visible way (the constructor). And while I highly value maintainability in code (even when it comes at the cost of conciseness), it still feels silly to have to pass around a logger everywhere, which I think is fine as a global. There are others ones too, such as globals.os, globals.platform, and globals.filesystem. For all non-test scenarios, there is going to be exactly one instance of these things, so passing them around is just noise outside of test code. I'm assuming this is why automatic DI systems like the ones in .NET, Spring, and Angular exist (just to name a few)1, though I am not highly experienced with these things. Still, as you said, it's very easy to forget to override these in the context of a test (one may not even realize that the class under test depends on a global).

Ideally, there would be some system where we use zones with default instances/generators that are not used in tests. Tests that access globals that are not overridden in the test declaration would throw an error when such a global is accessed. Could we have dart test (and thus flutter test) set some environment variable to indicate that we are running a test? Then, we can hook some logic somewhere to check that we are in a test and then throw if we access a global that was not overridden in the test.

If we implemented something like that, then I think we solve the gotchas that testing code with globals in them have. The only remaining drawbacks would be 1) the initial confusion that comes with zones, 2) it can take a new contributor slightly longer to figure out what a class is dependent on2, and 3) that the syntax for writing tests is a bit more bloated (e.g. like how testWithContext is now). I wonder if what @jonahwilliams was trying with the testBed thing in the past was similar at all? I don't know where to find that draft (edit: never mind I just realized this was linked to above).

Still, In the absence of such a system, which would take a non-trivial amount of time to implement (assuming it's even feasible), I do think manual DI is the way to go.

Footnotes

  1. edit: The problem domain of these frameworks vs this ad-hoc tool make this comparison a bit apple-to-oranges in hindsight

  2. Maybe we can do some sort of metaprogramming here now or in the future to alleviate this problem.

@christopherfujino
Copy link
Author

Could we have dart test (and thus flutter test) set some environment variable to indicate that we are running a test? Then, we can hook some logic somewhere to check that we are in a test and then throw if we access a global that was not overridden in the test.

I think checking for a magic env var in production code would be bad.

I'm not sure if we should do this, but I wonder if this could work:

  1. enforce (with some static test) that all getters in globals.dart NOT have a default fallback but either a ! operator or even better, some helper function that then throws a special error that encourages the user to file a bug telling us we messed up.
  2. Ensure every getter in globals.dart has an override provided in the context_runner (maybe we could also write a static test for this)
  3. Use testUsingContext as is today.

This would mean that at least we could be confident that any globals we have not explicitly provided that are used would be those defined in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/src/context.dart#L103.

This would have the following drawbacks:

  1. Unless/until we completely get rid of all DI, there's a risk that you may have multiple values of a singleton in your system, such as two loggers (one direct injected, and the anonymous default one constructed by testUsingContext). You may assert on the state of the one you direct injected, without knowing the behavior you're checking for happened to the anonymous one which you don't have a reference to.
  2. Your test may not actually be testing the behavior you're interested in, because it implicitly (without your knowing) overrode a global with a no-op instance of the class you actually modified.

Which makes me think actually, what about if we just removed all the defaults from testUsingContext? It would make test code verbose, but then at least production code wouldn't be.

@andrewkolos
Copy link

I'm not sure if we should do this, but I wonder if this could work:

  1. enforce (with some static test) that all getters in globals.dart NOT have a default fallback but either a ! operator or even better, some helper function that then throws a special error that encourages the user to file a bug telling us we messed up.
  2. Ensure every getter in globals.dart has an override provided in the context_runner (maybe we could also write a static test for this)
  3. Use testUsingContext as is today.

This would mean that at least we could be confident that any globals we have not explicitly provided that are used would be those defined in https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/src/context.dart#L103.

Unless I'm mistaken, this doesn't address the problem of being able to forget to provide a context override (e.g. accidentally using AndroidStudio.latestValid instead of providing a test instance). Still, enforcing the rule that all globals are provided through the context is certainly an idea I'd be interested in.

Which makes me think actually, what about if we just removed all the defaults from testUsingContext? It would make test code verbose, but then at least production code wouldn't be.

When you say "production code" are you referring only to https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/src/context.dart#L103? I don't see this as being significantly verbose.

@christopherfujino
Copy link
Author

christopherfujino commented Jun 9, 2023

Unless I'm mistaken, this doesn't address the problem of being able to forget to provide a context override (e.g. accidentally using AndroidStudio.latestValid instead of providing a test instance). Still, enforcing the rule that all globals are provided through the context is certainly an idea I'd be interested in.

Because of point #1, these would be runtime null check errors, right? And thus get caught when we run the test.

When you say "production code" are you referring only to https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/src/context.dart#L103? I don't see this as being significantly verbose.

No, I mean like the type of direct injection we would need to do to, for example, the BuildIOSCommand() constructor in order to completely eliminate all calls to globals.*.

@andrewkolos
Copy link

Unless I'm mistaken, this doesn't address the problem of being able to forget to provide a context override (e.g. accidentally using AndroidStudio.latestValid instead of providing a test instance). Still, enforcing the rule that all globals are provided through the context is certainly an idea I'd be interested in.

Because of point #1, these would be runtime null check errors, right? And thus get caught when we run the test.

Consider globals.androidStudio for example, which is defined like so:

AndroidStudio? get androidStudio => context.get<AndroidStudio>();

Say we want to run some test on ConfigCommand, which accesses globals.androidStudio.

If we forget to provide an override for AndroidStudio using the overrides parameter of testWithContext, then won't we silently use the default generator defined in context_runner.dart (which would look for Android Studio on the host system). This doesn't change if we add a null assertion operator (!) on the end (unless AndroidStudio.latestValid just happens to evaluate to null).

When you say "production code" are you referring only to https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/src/context.dart#L103? I don't see this as being significantly verbose.

No, I mean like the type of direct injection we would need to do to, for example, the BuildIOSCommand() constructor in order to completely eliminate all calls to globals.*.

I think I am failing to fundamentally understand the solution you are proposing, or we are focusing on different problems. As I understand it, your solution would solve the problem of a global being null when it shouldn't be1. I am trying to solve the problem of forgetting to--for example--provide an override for FileSystem when testing a class that uses globals.fs (in which case the test starts messing around with the user's filesystem). This is the biggest problem that our current zone-based DI system has, imo.

Let's discuss again next week.

Footnotes

  1. This wouldn't work out-of-the-box, because some globals have legitimate reasons for being null (e.g. java when the tool couldn't find a JDK on the host machine).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment