From 2cb13fcdce68911ecb81feb137f757d143271872 Mon Sep 17 00:00:00 2001 From: evanweible-wf Date: Fri, 10 Nov 2023 13:03:32 -0700 Subject: [PATCH 1/3] Add APIs to help test suggestors with resolved AST --- CHANGELOG.md | 7 ++ README.md | 46 ++++++++++ lib/test.dart | 126 +++++++++++++++++++++++--- test/ast_visiting_suggestor_test.dart | 39 ++++++++ 4 files changed, 205 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a6fccf..24a403f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## [1.2.0](https://github.com/Workiva/dart_codemod/compare/1.1.0...1.2.0) + +- Add `PackageContextForTest` to `package:codemod/test.dart` to help test +suggestors that require a fully resolved AST from the analyzer (for example: +suggestors using the `AstVisitingSuggestor` mixin with `shouldResolveAst` +enabled). + ## [1.1.0](https://github.com/Workiva/dart_codemod/compare/1.0.11...1.1.0) - Compatibility with Dart 3 and analyzer 6. diff --git a/README.md b/README.md index effe80e..56349b9 100644 --- a/README.md +++ b/README.md @@ -349,6 +349,52 @@ var foo = 'foo'; } ``` +### Testing Suggestors with Resolved AST + +The `fileContextForTest()` helper shown above makes it easy to test suggestors +that operate on the _unresolved_ AST, but some suggestors require the _resolved_ +AST. For example, a suggestor may need to rename a specific symbol from a specific +package, and so it would need to check the resolved element of a node. This is +only possible if the analysis context is aware of all the relevant files and +package dependencies. + +To help with this scenario, the `package:codemod/test.dart` library also exports +a `PackageContextForTest` helper class. This class handles creating a temporary +package directory, installing dependencies, and setting up an analysis context +that has access to the whole package and its dependencies. You can then add +source file(s) and use the wrapping `FileContext`s to test suggestors. + +```dart +import 'package:codemod/codemod.dart'; +import 'package:source_span/source_span.dart'; +import 'package:test/test.dart'; + +void main() { + group('AlwaysThrowsFixer', () { + test('returns Never instead', () async { + final pkg = await PackageContextForTest.fromPubspec('pkg', ''' +name: pkg +publish_to: none +environment: + sdk: '>=3.0.0 <4.0.0' +dependencies: + meta: ^1.0.0 +'''); + final context = await pkg.addFile('test.dart', ''' +import 'package:meta/meta.dart'; +@alwaysThrows toss() { throw 'Thrown'; } +'''); + final expectedOutput = ''' +import 'package:meta/meta.dart'; +Never toss() { throw 'Thrown'; } +'''; + expectSuggestorGeneratesPatches( + AlwaysThrowsFixer(), context, expectedOutput); + }); + }); +} +``` + ## References - [over_react_codemod][over_react_codemod]: codemods for the `over_react` UI diff --git a/lib/test.dart b/lib/test.dart index 2b6d5cd..ba69ca9 100644 --- a/lib/test.dart +++ b/lib/test.dart @@ -1,3 +1,5 @@ +import 'dart:io'; + import 'package:analyzer/dart/analysis/analysis_context_collection.dart'; import 'package:path/path.dart' as p; import 'package:test/test.dart'; @@ -9,7 +11,26 @@ import 'src/util.dart'; export 'src/util.dart' show applyPatches; -/// Creates a file with the given [name] and [sourceText] using the +/// Uses [suggestor] to generate a stream of patches for [context] and returns +/// what the resulting file contents would be after applying all of them. +/// +/// Use this to test that a suggestor produces the expected result: +/// test('MySuggestor', () async { +/// var context = await fileContextForTest('foo.dart', 'library foo;'); +/// var suggestor = MySuggestor(); +/// var expectedOutput = '...'; +/// expectSuggestorGeneratesPatches(suggestor, context, expectedOutput); +/// }); +void expectSuggestorGeneratesPatches( + Suggestor suggestor, FileContext context, dynamic resultMatcher) { + expect( + suggestor(context) + .toList() + .then((patches) => applyPatches(context.sourceFile, patches)), + completion(resultMatcher)); +} + +/// Creates a temporary file with the given [name] and [sourceText] using the /// `test_descriptor` package, sets up analysis for that file, and returns a /// [FileContext] wrapper around it. /// @@ -19,6 +40,9 @@ export 'src/util.dart' show applyPatches; /// var patches = MySuggestor().generatePatches(context); /// expect(patches, ...); /// }); +/// +/// See also: [PackageContextForTest] if testing [Suggestor]s that need a fully +/// resolved AST from the analyzer. Future fileContextForTest(String name, String sourceText) async { // Use test_descriptor to create the file in a temporary directory d.Descriptor descriptor; @@ -32,27 +56,103 @@ Future fileContextForTest(String name, String sourceText) async { await descriptor.create(); // Setup analysis for this file - final path = p.canonicalize(p.join(d.sandbox, name)); + final path = p.canonicalize(d.path(name)); final collection = AnalysisContextCollection(includedPaths: [path]); return FileContext(path, collection, root: d.sandbox); } -/// Uses [suggestor] to generate a stream of patches for [context] and returns -/// what the resulting file contents would be after applying all of them. +/// Creates a temporary directory with a pubspec using the `test_descriptor` +/// package, installs dependencies with `dart pub get`, and sets up an analysis +/// context for the package. /// -/// Use this to test that a suggestor produces the expected result: +/// Source files can then be added to the package with [addFile], which will +/// return a [FileContext] wrapper for use in tests. +/// +/// Use this to setup tests for [Suggestor]s that require the resolved AST, like +/// the [AstVisitingSuggestor] when `shouldResolveAst()` returns true. Doing so +/// will enable the analyzer to resolve imports and symbols from other source +/// files and dependencies. /// test('MySuggestor', () async { -/// var context = await fileContextForTest('foo.dart', 'library foo;'); +/// var pkg = await PackageContextForTest.fromPubspec('pkg', ''' +/// name: pkg +/// version: 0.0.0 +/// environment: +/// sdk: '>=3.0.0 <4.0.0' +/// dependencies: +/// meta: ^1.0.0 +/// '''); +/// var context = await pkg.addFile('foo.dart', ''' +/// import 'package:meta/meta.dart'; +/// @visibleForTesting var foo = true; +/// '''); /// var suggestor = MySuggestor(); /// var expectedOutput = '...'; /// expectSuggestorGeneratesPatches(suggestor, context, expectedOutput); /// }); -void expectSuggestorGeneratesPatches( - Suggestor suggestor, FileContext context, dynamic resultMatcher) { - expect( - suggestor(context) - .toList() - .then((patches) => applyPatches(context.sourceFile, patches)), - completion(resultMatcher)); +class PackageContextForTest { + final AnalysisContextCollection _collection; + final String _name; + final String _root; + + /// Creates a temporary directory named [dirName] using the `test_descriptor` + /// package, installs dependencies with `dart pub get`, sets up an analysis + /// context for the package, and returns a [PackageContextForTest] wrapper + /// that allows you to add source files to the package and use them in tests. + /// + /// Throws an [ArgumentError] if it fails to install dependencies. + static Future fromPubspec( + String dirName, + String pubspecContents, + ) async { + await d.dir(dirName, [ + d.file('pubspec.yaml', pubspecContents), + ]).create(); + + final root = p.canonicalize(d.path(dirName)); + final pubGet = + Process.runSync('dart', ['pub', 'get'], workingDirectory: root); + if (pubGet.exitCode != 0) { + printOnFailure(''' +PROCESS: dart pub get +WORKING DIR: $root +STDOUT: +${pubGet.stdout} +STDERR: +${pubGet.stderr} +'''); + throw ArgumentError('Failed to install dependencies from given pubspec'); + } + final collection = AnalysisContextCollection(includedPaths: [root]); + return PackageContextForTest._(dirName, root, collection); + } + + PackageContextForTest._(this._name, this._root, this._collection); + + /// Creates a temporary file at the given [path] (relative to the root of this + /// package) with the given [sourceText] using the `test_descriptor` package + /// and returns a [FileContext] wrapper around it. + /// + /// The returned [FileContext] will use the analysis context for this whole + /// package rather than just this file, which enables testing of [Suggestor]s + /// that require the resolved AST. + /// + /// See [PackageContextForTest] for an example. + Future addFile(String path, String sourceText) async { + // Use test_descriptor to create the file in a temporary directory + d.Descriptor descriptor; + final segments = p.split(path); + // Last segment should be the file + descriptor = d.file(segments.last, sourceText); + // Any preceding segments (if any) are directories + for (final dir in segments.reversed.skip(1)) { + descriptor = d.dir(dir, [descriptor]); + } + // Add the root directory. + descriptor = d.dir(_name, [descriptor]); + await descriptor.create(); + + final canonicalizedPath = p.canonicalize(p.join(d.sandbox, _name, path)); + return FileContext(canonicalizedPath, _collection, root: _root); + } } diff --git a/test/ast_visiting_suggestor_test.dart b/test/ast_visiting_suggestor_test.dart index 0432c8a..5d516ed 100644 --- a/test/ast_visiting_suggestor_test.dart +++ b/test/ast_visiting_suggestor_test.dart @@ -46,6 +46,24 @@ class LibNameDoubler extends RecursiveAstVisitor } } +class AlwaysThrowsFixer extends GeneralizingAstVisitor + with AstVisitingSuggestor { + @override + bool shouldResolveAst(_) => true; + + @override + void visitFunctionDeclaration(FunctionDeclaration node) { + for (final annotation in node.metadata) { + final isAlwaysThrows = annotation.name.name == 'alwaysThrows'; + final annotationPackage = annotation.element?.library?.identifier ?? ''; + final isFromPackageMeta = annotationPackage.startsWith('package:meta/'); + if (isAlwaysThrows && isFromPackageMeta) { + yieldPatch('Never', annotation.offset, annotation.end); + } + } + } +} + void main() { group('AstVisitingSuggestor', () { test('should get compilation unit, visit it, and yield patches', () async { @@ -99,5 +117,26 @@ void main() { expect(await patchesA.toList(), [Patch('aa', 8, 9)]); expect(await patchesC.toList(), [Patch('cc', 8, 9)]); }); + + test('should resolve AST and work with imports', () async { + final pkg = await PackageContextForTest.fromPubspec('pkg', ''' +name: pkg +publish_to: none +environment: + sdk: '>=2.19.0 <4.0.0' +dependencies: + meta: ^1.0.0 +'''); + final context = await pkg.addFile('test.dart', ''' +import 'package:meta/meta.dart'; +@alwaysThrows toss() { throw 'Thrown'; } +'''); + final expectedOutput = ''' +import 'package:meta/meta.dart'; +Never toss() { throw 'Thrown'; } +'''; + expectSuggestorGeneratesPatches( + AlwaysThrowsFixer(), context, expectedOutput); + }); }); } From 6633735ad0e2c4fd80e190a2cd108e819c1cff10 Mon Sep 17 00:00:00 2001 From: evanweible-wf Date: Fri, 10 Nov 2023 13:08:15 -0700 Subject: [PATCH 2/3] Ignore false positive dep validator result --- dart_dependency_validator.yaml | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 dart_dependency_validator.yaml diff --git a/dart_dependency_validator.yaml b/dart_dependency_validator.yaml new file mode 100644 index 0000000..fb84e6a --- /dev/null +++ b/dart_dependency_validator.yaml @@ -0,0 +1,3 @@ +ignore: + # Flagged as a false positive due to one of the unit test files + - meta From 5330958026ad8f526225adfbf9cfef010c1bd227 Mon Sep 17 00:00:00 2001 From: evanweible-wf Date: Fri, 10 Nov 2023 15:33:03 -0700 Subject: [PATCH 3/3] Make dir and file names optional when using PackageContextForTest --- README.md | 4 ++-- lib/test.dart | 24 +++++++++++++++++------- test/ast_visiting_suggestor_test.dart | 4 ++-- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 56349b9..a463ebc 100644 --- a/README.md +++ b/README.md @@ -372,7 +372,7 @@ import 'package:test/test.dart'; void main() { group('AlwaysThrowsFixer', () { test('returns Never instead', () async { - final pkg = await PackageContextForTest.fromPubspec('pkg', ''' + final pkg = await PackageContextForTest.fromPubspec(''' name: pkg publish_to: none environment: @@ -380,7 +380,7 @@ environment: dependencies: meta: ^1.0.0 '''); - final context = await pkg.addFile('test.dart', ''' + final context = await pkg.addFile(''' import 'package:meta/meta.dart'; @alwaysThrows toss() { throw 'Thrown'; } '''); diff --git a/lib/test.dart b/lib/test.dart index ba69ca9..3698c4e 100644 --- a/lib/test.dart +++ b/lib/test.dart @@ -74,7 +74,7 @@ Future fileContextForTest(String name, String sourceText) async { /// will enable the analyzer to resolve imports and symbols from other source /// files and dependencies. /// test('MySuggestor', () async { -/// var pkg = await PackageContextForTest.fromPubspec('pkg', ''' +/// var pkg = await PackageContextForTest.fromPubspec(''' /// name: pkg /// version: 0.0.0 /// environment: @@ -82,7 +82,7 @@ Future fileContextForTest(String name, String sourceText) async { /// dependencies: /// meta: ^1.0.0 /// '''); -/// var context = await pkg.addFile('foo.dart', ''' +/// var context = await pkg.addFile(''' /// import 'package:meta/meta.dart'; /// @visibleForTesting var foo = true; /// '''); @@ -94,17 +94,23 @@ class PackageContextForTest { final AnalysisContextCollection _collection; final String _name; final String _root; + static int _fileCounter = 0; + static int _packageCounter = 0; /// Creates a temporary directory named [dirName] using the `test_descriptor` /// package, installs dependencies with `dart pub get`, sets up an analysis /// context for the package, and returns a [PackageContextForTest] wrapper /// that allows you to add source files to the package and use them in tests. /// + /// If [dirName] is null, a unique name will be generated. + /// /// Throws an [ArgumentError] if it fails to install dependencies. static Future fromPubspec( - String dirName, - String pubspecContents, - ) async { + String pubspecContents, [ + String? dirName, + ]) async { + dirName ??= 'package_${_packageCounter++}'; + await d.dir(dirName, [ d.file('pubspec.yaml', pubspecContents), ]).create(); @@ -133,12 +139,16 @@ ${pubGet.stderr} /// package) with the given [sourceText] using the `test_descriptor` package /// and returns a [FileContext] wrapper around it. /// + /// If [path] is null, a unique filename will be generated. + /// /// The returned [FileContext] will use the analysis context for this whole /// package rather than just this file, which enables testing of [Suggestor]s /// that require the resolved AST. /// /// See [PackageContextForTest] for an example. - Future addFile(String path, String sourceText) async { + Future addFile(String sourceText, [String? path]) async { + path ??= 'test_${_fileCounter++}.dart'; + // Use test_descriptor to create the file in a temporary directory d.Descriptor descriptor; final segments = p.split(path); @@ -150,8 +160,8 @@ ${pubGet.stderr} } // Add the root directory. descriptor = d.dir(_name, [descriptor]); - await descriptor.create(); + await descriptor.create(); final canonicalizedPath = p.canonicalize(p.join(d.sandbox, _name, path)); return FileContext(canonicalizedPath, _collection, root: _root); } diff --git a/test/ast_visiting_suggestor_test.dart b/test/ast_visiting_suggestor_test.dart index 5d516ed..739b4dd 100644 --- a/test/ast_visiting_suggestor_test.dart +++ b/test/ast_visiting_suggestor_test.dart @@ -119,7 +119,7 @@ void main() { }); test('should resolve AST and work with imports', () async { - final pkg = await PackageContextForTest.fromPubspec('pkg', ''' + final pkg = await PackageContextForTest.fromPubspec(''' name: pkg publish_to: none environment: @@ -127,7 +127,7 @@ environment: dependencies: meta: ^1.0.0 '''); - final context = await pkg.addFile('test.dart', ''' + final context = await pkg.addFile(''' import 'package:meta/meta.dart'; @alwaysThrows toss() { throw 'Thrown'; } ''');