[ddc] Add rejection to hot reload test suite

Allows tests to define generation files that are expected to be
rejected as invalid for hot reload.

Tests generation files now support the extension `.reject.dart` to
signal a rejection is expected. A corresponding rejection error
message should be added to the map in the config.json file.

- Add new rejection logic to the file discovery and config file
  parsing.
- Add new reload receipts that are output by the runtime utils when
  running hot reload tests. These receipts are used to verify that
  each file generation was visited (not necessarily accepted) at
  runtime.

Change-Id: Ifee698f0d6502ecfceba2d5c733a2c88fb5daf7c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/394564
Commit-Queue: Nicholas Shahan <[email protected]>
Reviewed-by: Nate Biggs <[email protected]>

Author: nshahan

pkg/dev_compiler/test/hot_reload_suite.dart

@@ -158,18 +158,10 @@ self.\$dartReloadModifiedModules = async function(subAppName, callback) {
 }
 
 // Append a helper function for hot reload.
-self.\$injectedFilesAndLibrariesToReload = function() {
-  // Resolve the next generation's directory and load all modified files.
-  let nextGeneration = self.dartDevEmbedder.hotReloadGeneration + 1;
-  if (previousGenerations.has(nextGeneration)) {
-    throw Error('Fatal error: Previous generations are being re-run.');
-  }
-  previousGenerations.add(nextGeneration);
-  let modifiedFilePaths = modifiedFilesPerGeneration[nextGeneration];
-  // Stop if the next generation does not exist.
-  if (modifiedFilePaths == void 0) {
-    return;
-  }
+self.\$injectedFilesAndLibrariesToReload = function(fileGeneration) {
+  modifiedFilePaths = modifiedFilesPerGeneration[fileGeneration];
+  if (modifiedFilePaths == null) return null;
+  // Collect reload generation resources.
   let fileUrls = [];
   let libraryIds = [];
   for (let i = 0; i < modifiedFilePaths.length; i++) {
@@ -443,20 +435,12 @@ let _scriptUrls = {
 
     // Append hot reload runner-specific logic.
     let modifiedFilesPerGeneration = ${_encoder.convert(modifiedFilesPerGeneration)};
-    let previousGenerations = new Set();
 
-    self.\$injectedFilesAndLibrariesToReload = function() {
-      // Resolve the next generation's directory and load all modified files.
-      let nextGeneration = self.dartDevEmbedder.hotReloadGeneration + 1;
-      if (previousGenerations.has(nextGeneration)) {
-        throw Error('Fatal error: Previous generations are being re-run.');
-      }
-      previousGenerations.add(nextGeneration);
-      let modifiedFilePaths = modifiedFilesPerGeneration[nextGeneration];
-      // Stop if the next generation does not exist.
-      if (modifiedFilePaths == void 0) {
-        return;
-      }
+    // Append a helper function for hot reload.
+    self.\$injectedFilesAndLibrariesToReload = function(fileGeneration) {
+      modifiedFilePaths = modifiedFilesPerGeneration[fileGeneration];
+      if (modifiedFilePaths == null) return null;
+      // Collect reload generation resources.
       let fileUrls = [];
       let libraryIds = [];
       for (let i = 0; i < modifiedFilePaths.length; i++) {
@@ -468,6 +452,7 @@ let _scriptUrls = {
       return [fileUrls, libraryIds];
     }
 
+    let previousGenerations = new Set();
     self.\$dartReloadModifiedModules = async function(subAppName, callback) {
       let expectedName = "$entrypointModuleName";
       if (subAppName !== expectedName) {

pkg/reload_test/lib/ddc_helpers.dart

@@ -0,0 +1,54 @@
+// 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:convert';
+
+enum Status {
+  accepted,
+  rejected,
+  restarted;
+}
+
+/// Reports the result of a hot reload or restart at runtime for test validation
+/// purposes only.
+class HotReloadReceipt {
+  final int generation;
+  final Status status;
+
+  /// Optional message describing the reason this reload was rejected.
+  ///
+  /// Expected to be `null` when this reload was accepted.
+  final String? rejectionMessage;
+
+  static final _generationKey = 'generation';
+  static final _statusKey = 'status';
+  static final _rejectionMessageKey = 'rejectionMessage';
+
+  HotReloadReceipt({
+    required this.generation,
+    required this.status,
+    this.rejectionMessage,
+  });
+
+  @override
+  String toString() => jsonEncode(toJson());
+
+  HotReloadReceipt.fromJson(Map<String, dynamic> json)
+      : generation = json[_generationKey] as int,
+        status = Status.values.byName(json[_statusKey] as String),
+        rejectionMessage = json[_rejectionMessageKey] as String?;
+
+  Map<String, dynamic> toJson() {
+    return {
+      _generationKey: generation,
+      _statusKey: status.name,
+      if (rejectionMessage != null) _rejectionMessageKey: rejectionMessage,
+    };
+  }
+
+  static final hotReloadReceiptTag = '_!# HOT RELOAD RECEIPT #!_: ';
+
+  static final compileTimeErrorMessage =
+      'Correct error verified at compile time.';
+}

pkg/reload_test/lib/hot_reload_receipt.dart

@@ -7,8 +7,11 @@
 // The structure here reflects interfaces defined in DDC's
 // `ddc_module_loader.js`.
 
+import 'dart:convert';
 import 'dart:js_interop';
 
+import '../hot_reload_receipt.dart';
+
 extension type _DartLoader(JSObject _) implements JSObject {
   external _DDCLoader get loader;
 }
@@ -22,8 +25,8 @@ extension type _DDCLoader(JSObject _) implements JSObject {
 }
 
 extension type _DartDevEmbedder(JSObject _) implements JSObject {
-  external JSFunction hotReload;
-  external JSFunction hotRestart;
+  external JSPromise hotReload(JSArray<JSString> files, JSArray<JSString> ids);
+  external JSPromise hotRestart();
   external JSNumber get hotReloadGeneration;
   external JSNumber get hotRestartGeneration;
 }
@@ -32,7 +35,8 @@ extension type _DartDevEmbedder(JSObject _) implements JSObject {
 external _DartDevEmbedder get _dartDevEmbedder;
 
 @JS('\$injectedFilesAndLibrariesToReload')
-external JSFunction injectedFilesAndLibrariesToReload;
+external JSArray<JSArray<JSString>>? injectedFilesAndLibrariesToReload(
+    JSNumber requestedFileGeneration);
 
 @JS('\$dartLoader')
 external _DartLoader get _dartLoader;
@@ -43,15 +47,71 @@ int get hotRestartGeneration => _dartDevEmbedder.hotRestartGeneration.toDartInt;
 
 Future<void> hotRestart() async {
   _ddcLoader.intendedHotRestartGeneration++;
-  await (_dartDevEmbedder.hotRestart.callAsFunction() as JSPromise).toDart;
+  final restartReceipt = HotReloadReceipt(
+    generation: _ddcLoader.intendedHotRestartGeneration,
+    status: Status.restarted,
+  );
+  print('${HotReloadReceipt.hotReloadReceiptTag}'
+      '${jsonEncode(restartReceipt.toJson())}');
+  await _dartDevEmbedder.hotRestart().toDart;
 }
 
+/// The reload generation of the currently running application.
 int get hotReloadGeneration => _dartDevEmbedder.hotReloadGeneration.toDartInt;
 
-Future<void> hotReload() async {
-  var hotReloadArgs =
-      injectedFilesAndLibrariesToReload.callAsFunction() as JSArray;
-  await (_dartDevEmbedder.hotReload.callAsFunction(
-          null, hotReloadArgs[0], hotReloadArgs[1]) as JSPromise)
+/// The generation of reload requests by the test application.
+///
+/// This could differ from [hotReloadGeneration] when file generations get
+/// rejected and the running application stays in the previous "application"
+/// generation.
+int _hotReloadFileGeneration = 0;
+
+Future<void> hotReload({bool expectRejection = false}) async {
+  _hotReloadFileGeneration++;
+  final generationFileInfo =
+      injectedFilesAndLibrariesToReload(_hotReloadFileGeneration.toJS);
+  final HotReloadReceipt reloadStatus = expectRejection
+      ? _rejectNextGeneration(generationFileInfo)
+      : await _reloadNextGeneration(generationFileInfo);
+  // Write reload receipt with a leading tag to be recognized by the reload
+  // suite runner and validated.
+  print('${HotReloadReceipt.hotReloadReceiptTag}'
+      '${jsonEncode(reloadStatus.toJson())}');
+}
+
+HotReloadReceipt _rejectNextGeneration(
+    JSArray<JSArray<JSString>>? generationFileInfo) {
+  if (generationFileInfo != null) {
+    throw Exception(
+        'Generation $_hotReloadFileGeneration was not rejected at compile '
+        'time. Verify the calls of `hotReload(expectRejection: true)` in the '
+        'test source match the rejected generation files.');
+  }
+  // This reload wasn't expected to find any files so this is OK.
+  // * The correct reason for rejection was already validated at compile time
+  //   so nothing to check here.
+  // * Expected number and order of reloads are validated at the end of the test
+  //   run.
+  return HotReloadReceipt(
+    generation: _hotReloadFileGeneration,
+    status: Status.rejected,
+    rejectionMessage: HotReloadReceipt.compileTimeErrorMessage,
+  );
+}
+
+Future<HotReloadReceipt> _reloadNextGeneration(
+    JSArray<JSArray<JSString>>? generationFileInfo) async {
+  if (generationFileInfo == null) {
+    throw Exception(
+        'No compiled files found for generation $_hotReloadFileGeneration. '
+        'Verify the calls of `hotReload()` in the test match the accepted '
+        'generation source files.');
+  }
+  await (_dartDevEmbedder.hotReload(
+          generationFileInfo[0], generationFileInfo[1]))
       .toDart;
+  return HotReloadReceipt(
+    generation: _hotReloadFileGeneration,
+    status: Status.accepted,
+  );
 }

pkg/reload_test/lib/src/_ddc_reload_utils.dart

@@ -13,5 +13,5 @@ Future<void> hotRestart() async =>
 int get hotReloadGeneration =>
     throw Exception('Not implemented on this platform.');
 
-Future<void> hotReload() async =>
+Future<void> hotReload({bool expectRejection = false}) async =>
     throw Exception('Not implemented on this platform.');

pkg/reload_test/lib/src/_reload_utils_api.dart

@@ -4,11 +4,14 @@
 
 // Helper functions for the hot reload test suite.
 
+import 'dart:convert';
 import 'dart:developer' show Service;
 import 'dart:io' as io;
 import 'package:vm_service/vm_service.dart' show VmService, ReloadReport;
 import 'package:vm_service/vm_service_io.dart' as vm_service_io;
 
+import '../hot_reload_receipt.dart';
+
 int get hotRestartGeneration =>
     throw Exception('Not implemented on this platform.');
 
@@ -20,10 +23,19 @@ int get hotReloadGeneration => _reloadCounter;
 
 HotReloadHelper? _hotReloadHelper;
 
-Future<void> hotReload() async {
+Future<void> hotReload({bool expectRejection = false}) async {
   _hotReloadHelper ??= await HotReloadHelper.create();
-  _reloadCounter++;
-  await _hotReloadHelper!.reloadNextGeneration();
+  HotReloadReceipt reloadReceipt;
+  if (expectRejection) {
+    reloadReceipt = await _hotReloadHelper!._rejectNextGeneration();
+  } else {
+    _reloadCounter++;
+    reloadReceipt = await _hotReloadHelper!._reloadNextGeneration();
+  }
+  // Write reload receipt with a leading tag to be recognized by the reload
+  // suite runner and validated.
+  print('${HotReloadReceipt.hotReloadReceiptTag}'
+      '${jsonEncode(reloadReceipt.toJson())}');
 }
 
 /// Helper to mediate with the vm service protocol.
@@ -49,11 +61,17 @@ class HotReloadHelper {
   /// * All dill files have the same name across every generation
   final String dillName;
 
+  /// File name of a dill that contains compile time errors.
+  ///
+  /// We assume that this generation contained expected compile time errors and
+  /// should be rejected at runtime.
+  final String errorDillName;
+
   /// The current generation being executed by the VM.
   int generation = 0;
 
-  HotReloadHelper._(
-      this._vmService, this._id, this.testOutputDirUri, this.dillName);
+  HotReloadHelper._(this._vmService, this._id, this.testOutputDirUri,
+      this.dillName, this.errorDillName);
 
   /// Create a helper that is bound to the current VM and isolate.
   static Future<HotReloadHelper> create() async {
@@ -80,29 +98,92 @@ class HotReloadHelper {
       print('Error: Unable to find generation in dill file: $dillUri.');
       io.exit(1);
     }
+    final dillName = dillUri.pathSegments.last;
+    final errorDillName = dillName.replaceAll('.dill', '.error.dill');
 
     return HotReloadHelper._(
-        vmService, id, dillUri.resolve('../'), dillUri.pathSegments.last);
+        vmService, id, dillUri.resolve('../'), dillName, errorDillName);
   }
 
   /// Trigger a hot-reload on the current isolate for the next generation.
   ///
-  /// Also checks that the generation aftewards exists. If not, the VM service
+  /// Also checks that the generation afterwards exists. If not, the VM service
   /// is disconnected to allow the VM to complete.
-  Future<ReloadReport> reloadNextGeneration() async {
+  Future<HotReloadReceipt> _reloadNextGeneration() async {
     generation += 1;
     final nextGenerationDillUri =
         testOutputDirUri.resolve('generation$generation/$dillName');
     print('Reloading: $nextGenerationDillUri');
     var reloadReport = await _vmService.reloadSources(_id,
         rootLibUri: nextGenerationDillUri.path);
-    final nextNextGenerationDillUri =
-        testOutputDirUri.resolve('generation${generation + 1}/$dillName');
-    final hasNextNextGeneration =
-        io.File.fromUri(nextNextGenerationDillUri).existsSync();
-    if (!hasNextNextGeneration) {
-      await _vmService.dispose();
+    if (!reloadReport.success!) {
+      throw Exception('Reload for generation $generation was rejected.\n'
+          '${reloadReport.reasonForCancelling}');
+    }
+    var reloadReceipt = HotReloadReceipt(
+      generation: generation,
+      status: Status.accepted,
+    );
+    if (!hasNextGeneration) await cleanUp();
+    return reloadReceipt;
+  }
+
+  Future<HotReloadReceipt> _rejectNextGeneration() async {
+    generation += 1;
+    HotReloadReceipt reloadReceipt;
+    final errorDillFile = io.File.fromUri(
+        testOutputDirUri.resolve('generation$generation/$errorDillName'));
+    if (errorDillFile.existsSync()) {
+      // This generation contained a compile time error that has already been
+      // validated and should be rejected.
+      reloadReceipt = HotReloadReceipt(
+          generation: generation,
+          status: Status.rejected,
+          rejectionMessage: HotReloadReceipt.compileTimeErrorMessage);
+    } else {
+      final nextGenerationDillUri =
+          testOutputDirUri.resolve('generation$generation/$dillName');
+      print('Reloading (expecting rejection): $nextGenerationDillUri');
+      final reloadReport = await _vmService.reloadSources(_id,
+          rootLibUri: nextGenerationDillUri.path);
+      if (reloadReport.success!) {
+        throw Exception('Generation $generation was not rejected. Verify the '
+            'calls of `hotReload(expectRejection: true)` in the test source '
+            'match the rejected generation files.');
+      }
+      reloadReceipt = HotReloadReceipt(
+        generation: generation,
+        status: Status.rejected,
+        rejectionMessage: reloadReport.reasonForCancelling,
+      );
+    }
+    if (!hasNextGeneration) await cleanUp();
+    return reloadReceipt;
+  }
+
+  bool get hasNextGeneration {
+    final nextNextGenerationDirUri =
+        testOutputDirUri.resolve('generation${generation + 1}');
+    return io.Directory.fromUri(nextNextGenerationDirUri).existsSync();
+  }
+
+  Future<void> cleanUp() async => await _vmService.dispose();
+}
+
+/// Extension to expose the reason for a failed reload.
+///
+/// This is currently in the json response from the vm-service, but not exposed
+/// as an API in [ReloadReport].
+extension on ReloadReport {
+  String? get reasonForCancelling {
+    final notices = this.json?['notices'] as List?;
+    if (notices != null) {
+      for (final notice in notices) {
+        if (notice['type'] == 'ReasonForCancelling') {
+          return notice['message'] as String?;
+        }
+      }
     }
-    return reloadReport;
+    return null;
   }
 }