[dartdev] "dart language-server": Only skip --protocol=lsp if the --protocol flag is passed

... and not just any flag starting with "protocol".

I believe the original code used `startsWith()` to try and catch both `--protocol` as its own flag, and `--protocol=` when combined with the value. However, it also caught `--protocol-traffic-log`. `wasParsed()` should handle both of the first cases without the latter.

I also unskipped the tests for Windows because the skip was added in 44cee12 (Jan 2021) because the `deleteDirectory()` call failed on Windows (file locking) but f40b06a (Jan 2022) already fixed that.

Fixes #52501

Change-Id: I931a3f12dca2683167e999e7fe59bed18159eb73
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/441780
Auto-Submit: Danny Tuppeny <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Ben Konyi <[email protected]>

Author: DanTup

pkg/dartdev/lib/src/commands/language_server.dart

@@ -50,7 +50,7 @@ For more information about the server's capabilities and configuration, see:
     const lsp = server.Driver.PROTOCOL_LSP;
 
     var args = argResults!.arguments;
-    if (!args.any((arg) => arg.startsWith('--$protocol'))) {
+    if (!argResults!.wasParsed(protocol)) {
       args = [...args, '--$protocol=$lsp'];
     }
     try {

pkg/dartdev/test/commands/language_server_test.dart

@@ -6,6 +6,7 @@ import 'dart:async';
 import 'dart:convert';
 import 'dart:io';
 
+import 'package:path/path.dart' as path;
 import 'package:test/test.dart';
 
 import '../utils.dart' as utils;
@@ -17,19 +18,18 @@ void main() {
     'language-server',
     defineLanguageServerTests,
     timeout: utils.longTimeout,
-    onPlatform: {
-      'windows': Skip('https://github.com/dart-lang/sdk/issues/44679'),
-    },
   );
 }
 
 void defineLanguageServerTests() {
   late utils.TestProject project;
   Process? process;
 
-  Future<void> runWithLsp(List<String> args) async {
+  setUp(() {
     project = utils.project();
+  });
 
+  Future<void> runWithLsp(List<String> args) async {
     process = await project.start(args);
 
     // Send an LSP init.
@@ -45,6 +45,13 @@ void defineLanguageServerTests() {
       },
     });
 
+    // If we get stderr output (for example because we passed invalid args)
+    // then throw rather than waiting forever on stdout content that will never
+    // arrive.
+    process!.stderr.transform(utf8.decoder).listen((data) {
+      throw data.toString();
+    });
+
     process!.stdin.write('Content-Length: ${message.length}\r\n');
     process!.stdin.write('\r\n');
     process!.stdin.write(message);
@@ -65,14 +72,44 @@ void defineLanguageServerTests() {
     process = null;
   }
 
+  Future<void> runWithLegacy(List<String> args) async {
+    process = await project.start(args);
+
+    final Stream<String> inStream = process!.stdout
+        .transform<String>(utf8.decoder)
+        .transform<String>(const LineSplitter());
+
+    final line = await inStream.first;
+    final json = jsonDecode(line);
+
+    expect(json['event'], 'server.connected');
+    expect(json['params'], isNotNull);
+    final params = json['params'];
+    expect(params['version'], isNotEmpty);
+    expect(params['pid'], isNot(0));
+
+    process!.kill();
+    process = null;
+  }
+
   test('protocol default', () async {
     return runWithLsp(['language-server']);
   });
 
-  test('protocol lsp', () async {
+  test('protocol default with --protocol-log-file', () async {
+    // https://github.com/dart-lang/sdk/issues/52501
+    final logFile = path.join(project.dir.path, 'log.txt');
+    return runWithLsp(['language-server', '--protocol-traffic-log=$logFile']);
+  });
+
+  test('--protocol=lsp', () async {
     return runWithLsp(['language-server', '--protocol=lsp']);
   });
 
+  test('--protocol lsp', () async {
+    return runWithLsp(['language-server', '--protocol', 'lsp']);
+  });
+
   test('--use-aot-snapshot', () async {
     return runWithLsp(['language-server', '--use-aot-snapshot']);
   });
@@ -97,26 +134,12 @@ void defineLanguageServerTests() {
     ]);
   });
 
-  test('protocol analyzer', () async {
-    project = utils.project();
-
-    process = await project.start(['language-server', '--protocol=analyzer']);
-
-    final Stream<String> inStream = process!.stdout
-        .transform<String>(utf8.decoder)
-        .transform<String>(const LineSplitter());
-
-    final line = await inStream.first;
-    final json = jsonDecode(line);
-
-    expect(json['event'], 'server.connected');
-    expect(json['params'], isNotNull);
-    final params = json['params'];
-    expect(params['version'], isNotEmpty);
-    expect(params['pid'], isNot(0));
+  test('--protocol=analyzer', () async {
+    await runWithLegacy(['language-server', '--protocol=analyzer']);
+  });
 
-    process!.kill();
-    process = null;
+  test('--protocol analyzer', () async {
+    await runWithLegacy(['language-server', '--protocol', 'analyzer']);
   });
 }
 
@@ -138,6 +161,13 @@ Future<String> _readLspMessage(Stream<List<int>> stream) {
       // Check whether the buffer has a complete message.
       final bufferString = buffer.toString();
 
+      // If the buffer has what looks like the legacy banner, then just fail
+      // because we will never get an LSP message.
+      if (bufferString.contains('"event":"server.connected"')) {
+        completer.completeError(
+            'Expected LSP message but got legacy message: $bufferString');
+      }
+
       // To know if we have a complete message, we need to check we have the
       // headers, extract the content-length, then check we have that many
       // bytes in the body.