[dds/dap] Fix some races in termination of test runs in DAP tests

It's not guaranteed that a "terminate" request will get a response so this changes the helper to accept a "terminated" event as acknowledgement of termination too.

It also fixes some issues running individual tests that didn't set a CWD (by setting a default for tests, since we can't "dart run test:test" without a cwd), and also updates a breakpoint test to not send breakpoints on invalid lines (something that had been fixed in the CLI test, but not the Test test).

Fixes https://github.com/dart-lang/sdk/issues/53023

Change-Id: I19dd60eb2b6d56035bb4d5b02d8e90713a8ce42d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/315823
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>

pkg/dds/test/dap/integration/dart_test_test.dart

@@ -14,6 +14,8 @@ main() {
   late DapTestSession dap;
   setUp(() async {
     dap = await DapTestSession.setUp(additionalArgs: ['--test']);
+    // For "dart run test:test" to work we must always have a cwd set.
+    dap.client.defaultCwd = dap.testAppDir.path;
     await dap.addPackageDependency(dap.testAppDir, 'test');
   });
   tearDown(() => dap.tearDown());
@@ -59,7 +61,6 @@ main() {
         launch: () => client.launch(
           testFile.path,
           noDebug: true,
-          cwd: dap.testAppDir.path,
           args: ['--chain-stack-traces'], // to suppress warnings in the output
         ),
       );
@@ -81,7 +82,6 @@ main() {
         launch: () => client.launch(
           testFile.path,
           noDebug: true,
-          cwd: dap.testAppDir.path,
           // It's up to the calling IDE to pass the correct args for 'dart test'
           // if it wants to run a subset of tests.
           args: [
@@ -106,10 +106,7 @@ main() {
 
       // Collect output and test events while running the script.
       final outputEvents = await client.collectTestOutput(
-        launch: () => client.launch(
-          testFile.path,
-          cwd: dap.testAppDir.path,
-        ),
+        launch: () => client.launch(testFile.path),
       );
 
       // Collect paths from any OutputEvents that had them.
@@ -132,7 +129,6 @@ main() {
         start: () => client.hitBreakpoint(
           testFile,
           breakpointLine,
-          cwd: dap.testAppDir.path,
           args: ['--chain-stack-traces'], // to suppress warnings in the output
         ).then((stop) => client.continue_(stop.threadId!)),
       );
@@ -153,18 +149,11 @@ main() {
       final breakpointLine = lineWith(testFile, breakpointMarker);
 
       // Hit the breakpoint inside the test.
-      await client.hitBreakpoint(
-        testFile,
-        breakpointLine,
-        cwd: dap.testAppDir.path,
-      );
+      await client.hitBreakpoint(testFile, breakpointLine);
 
       // Send a single terminate, and expect a clean exit (with a `terminated`
       // event).
-      await Future.wait([
-        dap.client.event('terminated'),
-        dap.client.terminate(),
-      ], eagerError: true);
+      await client.terminate();
     });
 
     test('resolves and updates breakpoints', () async {
@@ -246,17 +235,12 @@ main() {
     test('responds to setBreakpoints before any breakpoint events', () async {
       final client = dap.client;
       final testFile =
-          dap.createTestFile(simpleTestBreakpointResolutionProgram);
+          dap.createTestFile(simpleTestBreakpointProgramWith50ExtraLines);
       final setBreakpointLine = lineWith(testFile, breakpointMarker);
 
       // Run the app and get to a breakpoint. This will allow us to add new
       // breakpoints in the same file that are _immediately_ resolved.
-      await Future.wait([
-        client.initialize(),
-        client.expectStop('breakpoint'),
-        client.setBreakpoint(testFile, setBreakpointLine),
-        client.launch(testFile.path),
-      ], eagerError: true);
+      await client.hitBreakpoint(testFile, setBreakpointLine);
 
       // Call setBreakpoint again, and ensure it response before we get any
       // breakpoint change events because we require their IDs before the change
@@ -274,7 +258,8 @@ main() {
             // sending requests to the VM to allow events to start coming back
             // from the VM before we complete. Without this, the test can pass
             // even without the fix.
-            .setBreakpoints(testFile, List.generate(50, (index) => index))
+            .setBreakpoints(testFile,
+                List.generate(50, (index) => setBreakpointLine + index))
             .then((_) => setBreakpointsResponded = true),
       ]);
     });

pkg/dds/test/dap/integration/debug_attach_test.dart

@@ -118,6 +118,7 @@ main() {
     });
 
     test('removes breakpoints/pause and resumes on detach', () async {
+      final client = dap.client;
       final testFile = dap.createTestFile(simpleBreakpointAndThrowProgram);
 
       final proc = await startDartProcessPaused(
@@ -130,9 +131,9 @@ main() {
       // Attach to the paused script without resuming and wait for the startup
       // pause event.
       await Future.wait([
-        dap.client.expectStop('entry'),
-        dap.client.start(
-          launch: () => dap.client.attach(
+        client.expectStop('entry'),
+        client.start(
+          launch: () => client.attach(
             vmServiceUri: vmServiceUri.toString(),
             autoResume: false,
             cwd: dap.testAppDir.path,
@@ -143,20 +144,17 @@ main() {
       // Set a breakpoint that we expect not to be hit, as detach should disable
       // it and resume.
       final breakpointLine = lineWith(testFile, breakpointMarker);
-      await dap.client.setBreakpoint(testFile, breakpointLine);
+      await client.setBreakpoint(testFile, breakpointLine);
 
       // Expect that termination is reported as 'Detached' when we explicitly
       // requested a detach.
-      expect(dap.client.outputEvents.map((output) => output.output.trim()),
+      expect(client.outputEvents.map((output) => output.output.trim()),
           emits('Detached.'));
 
       // Detach using terminateRequest. Despite the name, terminateRequest is
       // the request for a graceful detach (and disconnectRequest is the
       // forceful shutdown).
-      await Future.wait([
-        dap.client.event('terminated'),
-        dap.client.terminate(),
-      ]);
+      await client.terminate();
 
       // Expect the process terminates (and hasn't got stuck on the breakpoint
       // or exception).

pkg/dds/test/dap/integration/debug_test.dart

@@ -517,26 +517,24 @@ main() {
     });
 
     test('resolves URIs in tool events to file:///', () async {
+      final client = dap.client;
       final testFile =
           dap.createTestFile(simpleToolEventWithDartCoreUriProgram);
 
       // Capture the `dart.toolEvent` event.
-      final toolEventsFuture = dap.client.events('dart.toolEvent').first;
+      final toolEventsFuture = client.events('dart.toolEvent').first;
 
       // Run the script until we get the event (which means mapping has
       // completed).
       await Future.wait([
         toolEventsFuture,
-        dap.client.initialize(),
-        dap.client.launch(testFile.path),
+        client.initialize(),
+        client.launch(testFile.path),
       ], eagerError: true);
 
       // Terminate the app (since the test script has a delay to ensure it
       // doesn't terminate before the async mapping code completes).
-      await Future.wait([
-        dap.client.event('terminated'),
-        dap.client.terminate(),
-      ], eagerError: true);
+      await client.terminate();
 
       // Verify we got the right fileUri.
       final toolEvent = await toolEventsFuture;

pkg/dds/test/dap/integration/test_client.dart

@@ -55,6 +55,9 @@ class DapTestClient {
   /// All stderr OutputEvents that have occurred so far.
   final StringBuffer _stderr = StringBuffer();
 
+  /// A default cwd to use for all launches.
+  String? defaultCwd;
+
   DapTestClient._(
     this._channel,
     this._logger, {
@@ -143,11 +146,12 @@ class DapTestClient {
         ? expectStop('entry').then((event) => continue_(event.threadId!))
         : null;
 
+    cwd ??= defaultCwd;
     final attachResponse = sendRequest(
       DartAttachRequestArguments(
         vmServiceUri: vmServiceUri,
         vmServiceInfoFile: vmServiceInfoFile,
-        cwd: cwd,
+        cwd: cwd != null ? _normalizePath(cwd) : null,
         additionalProjectPaths: additionalProjectPaths,
         debugSdkLibraries: debugSdkLibraries,
         debugExternalPackageLibraries: debugExternalPackageLibraries,
@@ -299,6 +303,7 @@ class DapTestClient {
     bool? sendCustomProgressEvents,
     bool? allowAnsiColorOutput,
   }) {
+    cwd ??= defaultCwd;
     return sendRequest(
       DartLaunchRequestArguments(
         noDebug: noDebug,
@@ -444,9 +449,24 @@ class DapTestClient {
   /// Whether or not any `terminate()` request has been sent.
   bool get hasSentTerminateRequest => _hasSentTerminateRequest;
   bool _hasSentTerminateRequest = false;
-  Future<Response?> terminate() async {
+
+  /// Sends a terminate request.
+  ///
+  /// For convenience, returns a Future that completes when either this request
+  /// completes, or when a `terminated` event is received since it is not
+  /// guaranteed that this request will return a response during a shutdown.
+  Future<void> terminate() async {
     _hasSentTerminateRequest = true;
-    return sendRequest(TerminateArguments());
+    return Future.any([
+      event('terminated').then(
+        (event) => event,
+        // Ignore errors caused by this event not occurring before the stream
+        // closes as this is very possible if the application terminated just
+        // before this method was called.
+        onError: (_) => null,
+      ),
+      sendRequest(TerminateArguments()),
+    ]);
   }
 
   /// Sends a threads request to the server to request the list of active

pkg/dds/test/dap/integration/test_scripts.dart

@@ -282,6 +282,19 @@ const simpleTestBreakpointResolutionProgram = '''
   }
 ''';
 
+final simpleTestBreakpointProgramWith50ExtraLines = '''
+  import 'package:test/test.dart';
+
+  void main() {
+    group('group 1', () {
+      test('passing test', () async {
+        expect(1, equals(1)); $breakpointMarker
+        ${'await null;\n' * 50}
+      });
+    });
+  }
+''';
+
 /// A simple test that prints the numbers from 1 to 5.
 ///
 /// A breakpoint marker is on the line that prints '1' and the subsequent 4