From e2e6ee8b0264ef1809462033100b60fbeff148c0 Mon Sep 17 00:00:00 2001 From: Konstantin Shcheglov Date: Wed, 13 Mar 2024 01:18:58 +0000 Subject: [PATCH] Completion. Don't update @FailingTest expectations. Change-Id: I627a2b82fd663c56636f52699237380cbf98e750 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/357181 Commit-Queue: Konstantin Shcheglov Reviewed-by: Brian Wilkerson --- .../dart/location/object_pattern_test.dart | 14 +- .../completion/dart/text_expectations.dart | 245 +++++++++++++++--- 2 files changed, 207 insertions(+), 52 deletions(-) diff --git a/pkg/analysis_server/test/services/completion/dart/location/object_pattern_test.dart b/pkg/analysis_server/test/services/completion/dart/location/object_pattern_test.dart index 7973638415c..c58a457b6cd 100644 --- a/pkg/analysis_server/test/services/completion/dart/location/object_pattern_test.dart +++ b/pkg/analysis_server/test/services/completion/dart/location/object_pattern_test.dart @@ -20,18 +20,6 @@ class ObjectPatternTest extends AbstractCompletionDriverTest await super.setUp(); printerConfiguration.withLocationName = true; } - - @FailingTest(reason: 'Suggest invalid static field / getter') - @override - Future test_pattern_first() { - return super.test_pattern_first(); - } - - @FailingTest(reason: 'Suggest invalid static field / getter') - @override - Future test_pattern_second() { - return super.test_pattern_second(); - } } mixin ObjectPatternTestCases on AbstractCompletionDriverTest { @@ -509,6 +497,7 @@ suggestions '''); } + @FailingTest(reason: 'Suggest invalid static field / getter') Future test_pattern_first() async { await computeSuggestions(''' void f1(Object x0) { @@ -564,6 +553,7 @@ suggestions '''); } + @FailingTest(reason: 'Suggest invalid static field / getter') Future test_pattern_second() async { await computeSuggestions(''' void f1(Object x0) { diff --git a/pkg/analysis_server/test/services/completion/dart/text_expectations.dart b/pkg/analysis_server/test/services/completion/dart/text_expectations.dart index 74a5a33786a..b5c853ae602 100644 --- a/pkg/analysis_server/test/services/completion/dart/text_expectations.dart +++ b/pkg/analysis_server/test/services/completion/dart/text_expectations.dart @@ -4,66 +4,110 @@ import 'dart:io' as io; +import 'package:analyzer/dart/analysis/analysis_context_collection.dart'; +import 'package:analyzer/dart/analysis/results.dart'; +import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/file_system/physical_file_system.dart'; import 'package:analyzer/source/line_info.dart'; +import 'package:analyzer/src/dart/ast/ast.dart'; +import 'package:analyzer/src/utilities/extensions/collection.dart'; import 'package:test/test.dart'; import 'package:test_reflective_loader/test_reflective_loader.dart'; class TextExpectationsCollector { - static const _updatingIsEnabled = false; + /// If this flag is `true`, we accumulate updates to expectations. + /// This should only happen locally, to update tests or implementation. + /// + /// This flag should be `false` during code review. + static const updatingIsEnabled = true; - static const assertMethods = { - 'AbstractCompletionDriverTest.assertResponse', - 'SelectionTest._assertSelection', - }; + static final assertMethods = [ + _AssertMethod( + className: 'AbstractCompletionDriverTest', + methodName: 'assertResponse', + argument: _ArgumentIndex(0), + ), + _AssertMethod( + className: 'SelectionTest', + methodName: '_assertSelection', + argument: _ArgumentIndex(1), + ), + ]; static final Map _files = {}; static void add(String actual) { - if (!_updatingIsEnabled) { + if (!updatingIsEnabled) { return; } - var traceLines = '${StackTrace.current}'.split('\n'); - for (var traceIndex = 0; traceIndex < traceLines.length; traceIndex++) { - var traceLine = traceLines[traceIndex]; - for (var assertMethod in assertMethods) { - if (traceLine.contains(' $assertMethod ')) { - var invocationLine = traceLines[traceIndex + 1]; - var locationMatch = RegExp( - r'file://(.+_test.dart):(\d+):', - ).firstMatch(invocationLine); - if (locationMatch == null) { - fail('Cannot parse: $invocationLine'); - } + final traceLines = '${StackTrace.current}'.split('\n'); + for (final assertMethod in assertMethods) { + for (var traceIndex = 0; traceIndex < traceLines.length; traceIndex++) { + final traceLine = traceLines[traceIndex]; + if (!traceLine.contains(' ${assertMethod.stackTracePattern} ')) { + continue; + } - var path = locationMatch.group(1)!; - var line = int.parse(locationMatch.group(2)!); - var file = _getFile(path); + // Find the invocation of the assert method in the stack trace. + var invocationTraceIndex = traceIndex + 1; + if (traceLines[invocationTraceIndex] == '') { + invocationTraceIndex++; + } + final invocationTraceLine = traceLines[invocationTraceIndex]; - var invocationOffset = file.lineInfo.getOffsetOfLine(line - 1); + // Parse the invocation stack trace line. + final locationMatch = RegExp( + r'(file://.+_test.dart):(\d+):', + ).firstMatch(invocationTraceLine); + if (locationMatch == null) { + fail('Cannot parse: $invocationTraceLine'); + } - const String rawStringPrefix = "r'''"; - var expectationOffset = file.content.indexOf( - rawStringPrefix, - invocationOffset, - ); - expectationOffset += rawStringPrefix.length; - var expectationEnd = file.content.indexOf("'''", expectationOffset); + final path = Uri.parse(locationMatch.group(1)!).toFilePath(); + final line = int.parse(locationMatch.group(2)!); + final file = _getFile(path); - file.replacements.add( - _Replacement( - expectationOffset, - expectationEnd, - '\n$actual', - ), + final invocation = file.findInvocation( + invocationLine: line, + ); + if (invocation == null) { + fail('Cannot find MethodInvocation.'); + } + + if (invocation.methodName.name != assertMethod.methodName) { + fail( + 'Expected: ${assertMethod.methodName}\n' + 'Actual: ${invocation.methodName.name}\n', ); } + + if (invocation.isInFailingTest) { + return; + } + + final argumentList = invocation.argumentList; + final argument = assertMethod.argument.get(argumentList); + if (argument is! SimpleStringLiteral) { + fail('Not a literal: ${argument.runtimeType}'); + } + + file.addReplacement( + _Replacement( + argument.contentsOffset, + argument.contentsEnd, + actual, + ), + ); + + // Stop after the first (most specific) assert method. + return; } } } - static void _apply() { - for (var file in _files.values) { + static void apply() { + for (final file in _files.values) { file.applyReplacements(); } _files.clear(); @@ -77,22 +121,63 @@ class TextExpectationsCollector { @reflectiveTest class UpdateTextExpectations { test_applyReplacements() { - TextExpectationsCollector._apply(); + TextExpectationsCollector.apply(); } } +sealed class _Argument { + Expression get(ArgumentList argumentList); +} + +final class _ArgumentIndex extends _Argument { + final int index; + + _ArgumentIndex(this.index); + + @override + Expression get(ArgumentList argumentList) { + return argumentList.arguments + .whereNotType() + .elementAt(index); + } +} + +class _AssertMethod { + final String methodName; + final String stackTracePattern; + final _Argument argument; + + const _AssertMethod({ + required String className, + required this.methodName, + required this.argument, + }) : stackTracePattern = '$className.$methodName'; +} + class _File { final String path; final String content; final LineInfo lineInfo; + final CompilationUnit unit; final List<_Replacement> replacements = []; factory _File(String path) { - var content = io.File(path).readAsStringSync(); + final content = io.File(path).readAsStringSync(); + + final collection = AnalysisContextCollection( + resourceProvider: PhysicalResourceProvider.INSTANCE, + includedPaths: [path], + ); + final analysisContext = collection.contextFor(path); + final analysisSession = analysisContext.currentSession; + final parseResult = analysisSession.getParsedUnit(path); + parseResult as ParsedUnitResult; + return _File._( path: path, content: content, lineInfo: LineInfo.fromContent(content), + unit: parseResult.unit, ); } @@ -100,18 +185,83 @@ class _File { required this.path, required this.content, required this.lineInfo, + required this.unit, }); + void addReplacement(_Replacement replacement) { + // Check if there is the same replacement. + for (final existing in replacements) { + if (existing.offset == replacement.offset) { + // Sanity check. + if (existing.end != replacement.end) { + fail( + 'At offset: ${existing.offset}\n' + 'Existing end: ${existing.end}\n' + 'New end: ${replacement.end}\n', + ); + } + if (existing.text != replacement.text) { + fail( + 'At offset: ${existing.offset}\n' + 'Existing text:\n${existing.text}\n' + 'New text:\n${replacement.end}\n', + ); + } + // We already have the same replacement, exit. + return; + } + } + + // This is a new replacement, add it. + replacements.add(replacement); + } + void applyReplacements() { replacements.sort((a, b) => b.offset - a.offset); var newCode = content; - for (var replacement in replacements) { + for (final replacement in replacements) { newCode = newCode.substring(0, replacement.offset) + replacement.text + newCode.substring(replacement.end); } io.File(path).writeAsStringSync(newCode); } + + MethodInvocation? findInvocation({ + required int invocationLine, + }) { + final visitor = _InvocationVisitor( + lineInfo: lineInfo, + requestedLine: invocationLine, + ); + unit.accept(visitor); + return visitor.result; + } +} + +class _InvocationVisitor extends RecursiveAstVisitor { + final LineInfo lineInfo; + final int requestedLine; + MethodInvocation? result; + + _InvocationVisitor({ + required this.lineInfo, + required this.requestedLine, + }); + + @override + void visitMethodInvocation(MethodInvocation node) { + if (result != null) { + return; + } + + final nodeLine = lineInfo.getLocation(node.offset).lineNumber; + if (nodeLine == requestedLine) { + result = node; + } + + super.visitMethodInvocation(node); + } } class _Replacement { @@ -121,3 +271,18 @@ class _Replacement { _Replacement(this.offset, this.end, this.text); } + +extension on AstNode { + bool get isInFailingTest { + final testMethod = thisOrAncestorOfType(); + if (testMethod != null && testMethod.name.lexeme.startsWith('test_')) { + for (final annotation in testMethod.metadata) { + var name = annotation.name.name; + if (const {'FailingTest', 'failingTest'}.contains(name)) { + return true; + } + } + } + return false; + } +}