From de8aa39ff6611e161f190b91396cd207647a9cd7 Mon Sep 17 00:00:00 2001 From: Jens Johansen Date: Thu, 10 Oct 2024 12:41:16 +0000 Subject: [PATCH] [CFE] Fix leak testing after kernels ast.dart was split into parts The weekly bot this week finished in half the time, but was green. Turns out the splitting of ast.dart into parts made the actual leak testing not work because `Library` no longer existed in `ast.dart` (but rather in `src/ast/libraries.dart`). This CL: 1) Fixes the issue by also looking up the libraries uri (which is still `ast.dart`. 2) Adds an option for requiring to find instances of some things it looks for (e.g. `Library` in kernel) and throw if it doesn't. This would have made the weekly bot turn red (fail) instead of being green (saying that everything was fine) when really it wasn't. 3) Adds a test that is run on the try bots that will exercise the leak finding - and throw if it doesn't find `Library` in kernel. Change-Id: Ie69bfbd188eb870fdc1e340a341c1271187ef110 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/389162 Reviewed-by: Johnni Winther Commit-Queue: Jens Johansen --- pkg/front_end/test/fasta/suite_utils.dart | 7 ++ .../test/flutter_gallery_leak_tester.dart | 2 + .../test/spell_checking_list_tests.txt | 1 + .../test/vm_service_for_leak_detection.dart | 85 +++++++++------- .../test/vm_service_for_leak_smoke_test.dart | 21 ++++ .../test/vm_service_heap_helper.dart | 71 +++++++++---- .../test/vm_service_heap_helper_test.dart | 99 ++++++++++++++++++- pkg/testing/lib/src/chain.dart | 4 + pkg/testing/lib/src/run.dart | 3 +- 9 files changed, 232 insertions(+), 61 deletions(-) create mode 100644 pkg/front_end/test/vm_service_for_leak_smoke_test.dart diff --git a/pkg/front_end/test/fasta/suite_utils.dart b/pkg/front_end/test/fasta/suite_utils.dart index dad2103d678..e45b3dfbef9 100644 --- a/pkg/front_end/test/fasta/suite_utils.dart +++ b/pkg/front_end/test/fasta/suite_utils.dart @@ -14,6 +14,8 @@ import 'package:testing/testing.dart' show Step, TestDescription; import '../coverage_helper.dart'; import 'testing/suite.dart'; +const String limitToArgument = "--limitTo="; + Future internalMain( CreateContext createContext, { List arguments = const [], @@ -25,6 +27,7 @@ Future internalMain( Logger logger = const StdoutLogger(); List? argumentsTrimmed; Uri? coverageUri; + int? limitTo; for (int i = 0; i < arguments.length; i++) { String argument = arguments[i]; bool trimmed = false; @@ -42,6 +45,9 @@ Future internalMain( // Have this 1-indexed when given as an input. shard = int.parse(argument.substring("--shard=".length)) - 1; trimmed = true; + } else if (argument.startsWith(limitToArgument)) { + limitTo = int.tryParse(argument.substring(limitToArgument.length)); + trimmed = true; } if (trimmed && argumentsTrimmed == null) { @@ -63,6 +69,7 @@ Future internalMain( configurationPath: configurationPath ?? "../../testing.json", shards: shards, shard: shard, + limitTo: limitTo, logger: logger, ); if (coverageUri != null) { diff --git a/pkg/front_end/test/flutter_gallery_leak_tester.dart b/pkg/front_end/test/flutter_gallery_leak_tester.dart index 388e4f0d800..6f7019db5d2 100644 --- a/pkg/front_end/test/flutter_gallery_leak_tester.dart +++ b/pkg/front_end/test/flutter_gallery_leak_tester.dart @@ -158,6 +158,7 @@ Future main(List args) async { Uri.parse("package:kernel/ast.dart"), "Library", ["fileUri"], + expectToAlwaysFind: true, )); helper.VMServiceHeapHelperSpecificExactLeakFinder heapHelper = new helper.VMServiceHeapHelperSpecificExactLeakFinder( @@ -167,6 +168,7 @@ Future main(List args) async { Uri.parse("package:kernel/ast.dart"), "Library", ["fileUri", "libraryIdForTesting"], + expectToAlwaysFind: true, ), ], throwOnPossibleLeak: true, diff --git a/pkg/front_end/test/spell_checking_list_tests.txt b/pkg/front_end/test/spell_checking_list_tests.txt index a4e59f26ca1..26e75a09be6 100644 --- a/pkg/front_end/test/spell_checking_list_tests.txt +++ b/pkg/front_end/test/spell_checking_list_tests.txt @@ -437,6 +437,7 @@ ing inhibit inlinable inlineable +inoperable insights instrument instrumenter diff --git a/pkg/front_end/test/vm_service_for_leak_detection.dart b/pkg/front_end/test/vm_service_for_leak_detection.dart index 225994ba83f..87b15240b54 100644 --- a/pkg/front_end/test/vm_service_for_leak_detection.dart +++ b/pkg/front_end/test/vm_service_for_leak_detection.dart @@ -7,43 +7,9 @@ import 'dart:io'; import "vm_service_heap_helper.dart" as helper; Future main(List args) async { - List interests = []; - interests.add(new helper.Interest( - Uri.parse("package:front_end/src/source/source_library_builder.dart"), - "SourceLibraryBuilder", - ["fileUri"], - )); - interests.add(new helper.Interest( - Uri.parse("package:front_end/src/source/source_extension_builder.dart"), - "SourceExtensionBuilder", - ["extension"], - )); - interests.add(new helper.Interest( - Uri.parse("package:kernel/ast.dart"), - "Library", - ["fileUri"], - )); - interests.add(new helper.Interest( - Uri.parse("package:kernel/ast.dart"), - "Extension", - ["name", "fileUri"], - )); - - helper.VMServiceHeapHelperSpecificExactLeakFinder createNewLeakFinder() => - new helper.VMServiceHeapHelperSpecificExactLeakFinder( - interests: interests, - prettyPrints: [ - new helper.Interest( - Uri.parse("package:kernel/ast.dart"), - "Library", - ["fileUri", "libraryIdForTesting"], - ), - ], - throwOnPossibleLeak: true, - ); - + List interests = getInterests(); helper.VMServiceHeapHelperSpecificExactLeakFinder heapHelper = - createNewLeakFinder(); + createNewLeakFinder(interests); if (args.length > 0 && args[0] == "--dart2js") { await heapHelper.start([ @@ -79,7 +45,7 @@ Future main(List args) async { rethrow; } print("Will retry in a few seconds."); - heapHelper = createNewLeakFinder(); + heapHelper = createNewLeakFinder(interests); await Future.delayed(const Duration(seconds: 2)); } } @@ -100,7 +66,7 @@ Future main(List args) async { rethrow; } print("Will retry in a few seconds."); - heapHelper = createNewLeakFinder(); + heapHelper = createNewLeakFinder(interests); await Future.delayed(const Duration(seconds: 2)); } } @@ -114,3 +80,46 @@ Future main(List args) async { ]); } } + +helper.VMServiceHeapHelperSpecificExactLeakFinder createNewLeakFinder( + List interests) { + return new helper.VMServiceHeapHelperSpecificExactLeakFinder( + interests: interests, + prettyPrints: [ + new helper.Interest( + Uri.parse("package:kernel/ast.dart"), + "Library", + ["fileUri", "libraryIdForTesting"], + expectToAlwaysFind: true, + ), + ], + throwOnPossibleLeak: true, + ); +} + +List getInterests() { + List interests = []; + interests.add(new helper.Interest( + Uri.parse("package:front_end/src/source/source_library_builder.dart"), + "SourceLibraryBuilder", + ["fileUri"], + )); + interests.add(new helper.Interest( + Uri.parse("package:front_end/src/source/source_extension_builder.dart"), + "SourceExtensionBuilder", + ["extension"], + )); + interests.add(new helper.Interest( + Uri.parse("package:kernel/ast.dart"), + "Library", + ["fileUri"], + expectToAlwaysFind: true, + )); + interests.add(new helper.Interest( + Uri.parse("package:kernel/ast.dart"), + "Extension", + ["name", "fileUri"], + expectToAlwaysFind: true, + )); + return interests; +} diff --git a/pkg/front_end/test/vm_service_for_leak_smoke_test.dart b/pkg/front_end/test/vm_service_for_leak_smoke_test.dart new file mode 100644 index 00000000000..66eaa2bf812 --- /dev/null +++ b/pkg/front_end/test/vm_service_for_leak_smoke_test.dart @@ -0,0 +1,21 @@ +// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'dart:io'; + +import 'fasta/suite_utils.dart' show limitToArgument; +import "vm_service_for_leak_detection.dart" as helper; + +Future main(List args) async { + /// Run just a single test from the incremental suite (with finding leaks) to + /// verify that leak finding actually works and that - e.g. a move of kernel + /// stuff - makes it inoperable. Note that we require, for instance, + /// the Library class in the kernel ast to be found. + await helper.createNewLeakFinder(helper.getInterests()).start([ + "--enable-asserts", + Platform.script.resolve("incremental_suite.dart").toString(), + "${limitToArgument}1", + "-DaddDebugBreaks=true", + ]); +} diff --git a/pkg/front_end/test/vm_service_heap_helper.dart b/pkg/front_end/test/vm_service_heap_helper.dart index 66322d7a642..4f112eb1f66 100644 --- a/pkg/front_end/test/vm_service_heap_helper.dart +++ b/pkg/front_end/test/vm_service_heap_helper.dart @@ -7,10 +7,9 @@ import "vm_service_helper.dart" as vmService; class VMServiceHeapHelperSpecificExactLeakFinder extends vmService.LaunchingVMServiceHelper { final Set _interestsClassNames = {}; - final Map>> _interests = - new Map>>(); - final Map>> _prettyPrints = - new Map>>(); + final Map>> _interests = {}; + final Map>> _prettyPrints = {}; + final Set _shouldAlwaysFind = {}; final bool throwOnPossibleLeak; bool verbose = false; int? timeout; @@ -22,6 +21,9 @@ class VMServiceHeapHelperSpecificExactLeakFinder }) { if (interests.isEmpty) throw "Empty list of interests given"; for (Interest interest in interests) { + if (interest.expectToAlwaysFind) { + _shouldAlwaysFind.add("${interest.uri}|${interest.className}"); + } Map>? classToFields = _interests[interest.uri]; if (classToFields == null) { classToFields = Map>(); @@ -124,28 +126,34 @@ class VMServiceHeapHelperSpecificExactLeakFinder stopwatch.reset(); List leaks = []; + Set leftToAlwaysFind = _shouldAlwaysFind.toSet(); for (vmService.ClassHeapStats member in allocationProfile.members!) { if (_interestsClassNames.contains(member.classRef!.name)) { + String? libraryId = member.classRef?.library?.id; + if (libraryId == null) continue; + vmService.Library library = (await serviceClient.getObject( + _isolateRef.id!, libraryId)) as vmService.Library; + String? importUriString = library.uri; + if (importUriString == null) continue; + String? classId = member.classRef?.id; + if (classId == null) continue; vmService.Class c = (await serviceClient.getObject( - _isolateRef.id!, member.classRef!.id!)) as vmService.Class; - String? uriString = c.location?.script?.uri; - if (uriString == null) continue; - Uri uri = Uri.parse(uriString); - Map>? uriInterest = _interests[uri]; - if (uriInterest == null) continue; - List? fieldsForClass = uriInterest[c.name]; - if (fieldsForClass == null) continue; + _isolateRef.id!, classId)) as vmService.Class; + String? partUriString = c.location?.script?.uri; + if (partUriString == null) continue; + String? className = c.name; + if (className == null) continue; - List fieldsForClassPrettyPrint = fieldsForClass; - - uriInterest = _prettyPrints[uri]; - if (uriInterest != null) { - if (uriInterest[c.name] != null) { - fieldsForClassPrettyPrint = uriInterest[c.name]!; - } - } + (List, List)? fieldsData = + _getFieldsForClassAndPrettyPrint(partUriString, className) ?? + _getFieldsForClassAndPrettyPrint(importUriString, className); + if (fieldsData == null) continue; + List fieldsForClass = fieldsData.$1; + List fieldsForClassPrettyPrint = fieldsData.$2; if (member.instancesCurrent != 0) { + leftToAlwaysFind.remove("$partUriString|$className"); + leftToAlwaysFind.remove("$importUriString|$className"); if (verbose) { print("Has ${member.instancesCurrent} instances of " "${member.classRef!.name}"); @@ -166,6 +174,9 @@ class VMServiceHeapHelperSpecificExactLeakFinder } else { noLeakDetected(); } + if (leftToAlwaysFind.isNotEmpty) { + throw "Expected to find, but didn't: $leftToAlwaysFind"; + } print("Looked for leaks in ${stopwatch.elapsedMilliseconds} ms"); @@ -181,6 +192,22 @@ class VMServiceHeapHelperSpecificExactLeakFinder } } + (List, List)? _getFieldsForClassAndPrettyPrint( + String uriString, String className) { + Uri uri = Uri.parse(uriString); + Map>? uriInterest = _interests[uri]; + if (uriInterest == null) return null; + List? fieldsForClass = uriInterest[className]; + if (fieldsForClass == null) return null; + + List fieldsForClassPrettyPrint = fieldsForClass; + uriInterest = _prettyPrints[uri]; + if (uriInterest != null && uriInterest[className] != null) { + fieldsForClassPrettyPrint = uriInterest[className]!; + } + return (fieldsForClass, fieldsForClassPrettyPrint); + } + Future> _findLeaks( vmService.IsolateRef isolateRef, vmService.ClassRef classRef, @@ -364,8 +391,10 @@ class Interest { final Uri uri; final String className; final List fieldNames; + final bool expectToAlwaysFind; - Interest(this.uri, this.className, this.fieldNames); + Interest(this.uri, this.className, this.fieldNames, + {this.expectToAlwaysFind = false}); } class Leak { diff --git a/pkg/front_end/test/vm_service_heap_helper_test.dart b/pkg/front_end/test/vm_service_heap_helper_test.dart index 3879d8894da..fc1f03f2fa8 100644 --- a/pkg/front_end/test/vm_service_heap_helper_test.dart +++ b/pkg/front_end/test/vm_service_heap_helper_test.dart @@ -15,6 +15,14 @@ Future main(List args) async { return doLeak(); } + await findsExpectedLeakData(); + await doesNotThrowsWhenNotFindingIfShouldNotAlwaysBeFound(); + await throwsWhenNotFindingWhatShouldAlwaysBeFound(); + + print("Done!"); +} + +Future findsExpectedLeakData() async { List interests = []; interests.add( new helper.Interest( @@ -37,6 +45,7 @@ Future main(List args) async { Platform.script, "LeakMe", ["unique", "forPrettyPrinting"], + expectToAlwaysFind: true, ), new helper.Interest( Platform.script, @@ -78,8 +87,96 @@ Future main(List args) async { "vs\n\n" "- ${leakData.join("\n- ")}"; } +} - print("Done!"); +Future doesNotThrowsWhenNotFindingIfShouldNotAlwaysBeFound() async { + List interests = []; + interests.add( + // A class called "LeakMeSpellingError" doesn't exist. But we don't say we + // expect to find it so it doesn't throw. + new helper.Interest( + Platform.script, + "LeakMeSpellingError", + ["unique"], + ), + ); + LeakFinderTest heapHelper = new LeakFinderTest( + interests: interests, + prettyPrints: const [], + throwOnPossibleLeak: false, + ); + + List<({dynamic error, StackTrace st})> errors = + await runAndGetErrors(() async { + await heapHelper.start( + [ + "--enable-asserts", + Platform.script.toString(), + "--leak", + ], + stderrReceiver: (s) {}, + stdoutReceiver: (s) {}, + ); + return heapHelper.completer.future; + }); + + // Run is now over. Verify we got the wanted error. + if (errors.length != 0) { + throw "Expected 0 error, got ${errors.length}: $errors"; + } +} + +Future throwsWhenNotFindingWhatShouldAlwaysBeFound() async { + List interests = []; + interests.add( + // Expect to find a class called "LeakMeSpellingError" --- but it doesn't + // exist so it won't. It should thus throw because "expectToAlwaysFind" is + // true. + new helper.Interest( + Platform.script, + "LeakMeSpellingError", + ["unique"], + expectToAlwaysFind: true, + ), + ); + LeakFinderTest heapHelper = new LeakFinderTest( + interests: interests, + prettyPrints: const [], + throwOnPossibleLeak: false, + ); + + List<({dynamic error, StackTrace st})> errors = + await runAndGetErrors(() async { + await heapHelper.start( + [ + "--enable-asserts", + Platform.script.toString(), + "--leak", + ], + stderrReceiver: (s) {}, + stdoutReceiver: (s) {}, + ); + return heapHelper.completer.future; + }); + + // Run is now over. Verify we got the wanted error. + if (errors.length != 1 || + !errors[0].error.toString().contains("Expected to find, but didn't")) { + throw "Expected 1 error, got ${errors.length}: $errors"; + } +} + +Future> runAndGetErrors( + Future Function() f) async { + List<({dynamic error, StackTrace st})> errors = []; + await runZoned(() { + return f(); + }, zoneSpecification: ZoneSpecification( + handleUncaughtError: (self, parent, zone, error, stackTrace) { + errors.add((error: error, st: stackTrace)); + }, + )); + return errors; } void doLeak() { diff --git a/pkg/testing/lib/src/chain.dart b/pkg/testing/lib/src/chain.dart index aead8550835..55037c4a148 100644 --- a/pkg/testing/lib/src/chain.dart +++ b/pkg/testing/lib/src/chain.dart @@ -118,6 +118,7 @@ abstract class ChainContext { Future run(Chain suite, Set selectors, {int shards = 1, int shard = 0, + int? limitTo, Logger logger = const StdoutLogger()}) async { assert(shards >= 1, "Invalid shards count: $shards"); assert(0 <= shard && shard < shards, @@ -147,6 +148,9 @@ abstract class ChainContext { } descriptions = shardDescriptions; } + if (limitTo != null && limitTo > 0 && limitTo < descriptions.length) { + descriptions = descriptions.sublist(0, limitTo); + } Map unexpectedResults = {}; Map> unexpectedOutcomes = diff --git a/pkg/testing/lib/src/run.dart b/pkg/testing/lib/src/run.dart index bd5e66efe5e..cb3e571f3f2 100644 --- a/pkg/testing/lib/src/run.dart +++ b/pkg/testing/lib/src/run.dart @@ -51,6 +51,7 @@ Future runMe(List arguments, CreateContext f, Uri? me, int shards = 1, int shard = 0, + int? limitTo, Logger logger = const StdoutLogger()}) { me ??= Platform.script; return withErrorHandling(() async { @@ -61,7 +62,7 @@ Future runMe(List arguments, CreateContext f, if (me == suite.source) { ChainContext context = await f(suite, cl.environment); await context.run(suite, Set.from(cl.selectors), - shards: shards, shard: shard, logger: logger); + shards: shards, shard: shard, limitTo: limitTo, logger: logger); } } }, logger: logger);