DAS plugins - rule testing - simpler setup for stubbing a package

[srawlins]

Right now a (lint) rule test can stub out a package in the setUp method like so:

  void setUp() {
    super.setUp();

    var matcherPath = '/packages/matcher';
    newFile('$matcherPath/lib/matcher.dart', '''
void expect(dynamic actual, dynamic matcher) {}

const isNotNull = 0;
const isNull = 0;
''');
    writeTestPackageConfig(
      PackageConfigFileBuilder()
        ..add(name: 'matcher', rootPath: convertPath(matcherPath)),
    );
  }

This could be made simpler, without much complexity. But it might be nice to discuss design for a moment.

The basic idea is that certain lint rules might check things like "Is this SimpleIdentifier spelled 'expect'? And is this SimpleIdentifier from the 'matcher' package?". And those checks should work on mock/stub packages (in a test) or on real ones. And test code needs to be able to refer to 'expect' from the 'matcher' package.

So you want to be able to write one or more files in a separate package, such that those files can be imported with a package: syntax. And those files should be written to file system, and also the package should be added to the test package config.

[srawlins]

CC @bwilkerson

Here's one idea:

  void setUp() {
    newPackageFile('matcher', 'lib/matcher.dart', '''
void expect(dynamic actual, dynamic matcher) {}

const isNotNull = 0;
const isNull = 0;
''');

    super.setUp();
  }

• The first argument is the package name.
• The second argument is the file path, relative to the package root; I think it's good to keep the 'lib/' there, even though it will almost always start with 'lib/'. Without the 'lib/', it's ambiguous, at a glance, where the file is relative to; but with the leading `'lib/', it's quite unambiguous; also if we decided that the path is relative to 'lib/', it would be extremely breaking/hard to make a change to support writing files outside 'lib/', if we ever wanted to do so.
• Then the file content; similar API to the newFile() method.
• Users don't need to pick a spot in the filesystem (/packages/matcher); this is good as it is irrelevant.
• Users don't need to know about PackageConfigFileBuilder, or all of the boilerplate passing around the same package name here and there.
• Behind the scenes, PubPackageResolutionTest could keep track of the package names added via newPackageFile(), and then add them all, when writing the package config in the PubPackageResolutionTest setUp method.

[bwilkerson]

I like this, but it has one limitation that I'm not sure we want, which is that you can only add one file to the package. For most cases that's probably fine, but it means that larger packages are hard to mock (requiring falling back on the old API).

One minor variation would be to use a builder-like pattern. That might look something like

  void setUp() {
    newPackage('matcher')
      ..addFile('lib/matcher.dart', '''
void expect(dynamic actual, dynamic matcher) {}

const isNotNull = 0;
const isNull = 0;
''')
      ..addFile('lib/other.dart', '''
// some other code
''');

    super.setUp();
  }

I considered using a map rather than individual method calls, but I think that's a little harder to read, despite being shorter:

  void setUp() {
    newPackage('matcher', {
      'lib/matcher.dart': '''
void expect(dynamic actual, dynamic matcher) {}

const isNotNull = 0;
const isNull = 0;
''',
      'lib/other.dart': '''
// some other code
''',
    });

    super.setUp();
  }

Maybe I'm overthinking this.

[srawlins]

I like this, but it has one limitation that I'm not sure we want, which is that you can only add one file to the package.

Why is that? You could call newPackageFile for multiple files in the same package. The package name is stored once, so that super.setUp adds the package name and path to the package config.

[bwilkerson]

My assumption was that either it wouldn't be allowed or the package name would be added multiple times, causing problems. I didn't check the implementation. :-)

But even so, having multiple invocations of newPackageFile per package seems less than ideal.

• You have to repeat the package name for each file you want to add.
• If there's a typo in the package name in one invocation it could be hard to spot the reason for a test failure (or realize that the test isn't testing what you thought it was).
• If two intended-to-be-different packages were accidentally given the same name it would also be hard to notice.

It seems like the builder approach would be less error-prone.

[srawlins]

Yeah those are good points. I like your builder-style API; it's still very succinct!