Create Hot Restart over websocket test #173852
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new integration test for hot restart over a WebSocket connection and refactors the existing hot reload test to use a new shared utility file, websocket_dwds_test_common.dart. The refactoring is well done and improves code reuse. I've identified a potential resource leak in the new common utility file where a Chrome process might not be cleaned up on certain failures. I've also pointed out a few places where documentation is missing for public members, which violates the project's style guide.
packages/flutter_tools/test/integration.shard/test_data/websocket_dwds_test_common.dart
Outdated
Show resolved
Hide resolved
| final Duration debugUrlTimeout; | ||
| final Duration appStartTimeout; |
There was a problem hiding this comment.
Public fields should have documentation comments. 1
/// The maximum time to wait for the DWDS debug URL to be available.
final Duration debugUrlTimeout;
/// The maximum time to wait for the app to start after Chrome connects.
final Duration appStartTimeout;
Style Guide References
Footnotes
| final StringBuffer stdout; | ||
| final io.Process chromeProcess; | ||
| final StreamSubscription<String> subscription; |
There was a problem hiding this comment.
Public fields should have documentation comments. 1
/// The captured stdout from the Flutter process.
final StringBuffer stdout;
/// The headless Chrome process.
final io.Process chromeProcess;
/// The subscription to the stdout stream of the Flutter process.
final StreamSubscription<String> subscription;
Style Guide References
Footnotes
| final StreamSubscription<String> subscription = flutter.stdout.listen((String e) { | ||
| stdout.writeln(e); | ||
| // Extract the debug connection URL | ||
| if (e.contains('Waiting for connection from Dart debug extension at http://')) { |
There was a problem hiding this comment.
If we have the regexp we can just use firstMatch and it'll be null if there isn't a match. It's harder to keep 2 copies of this string in sync.
| r'Waiting for connection from Dart debug extension at (http://[^\s]+)', | ||
| ); | ||
| final Match? match = debugUrlPattern.firstMatch(e); | ||
| if (match != null && !sawDebugUrl.isCompleted) { |
There was a problem hiding this comment.
Is it possible to see the debug URL twice? Seems like something is off if that happens so perhaps we should throw a clear error. Otherwise it'll just look like a timeout.
|
|
||
| io.Process? chromeProcess; | ||
| try { | ||
| // Step 1: Start Flutter app with web-server device (will wait for debug connection) |
There was a problem hiding this comment.
Comment step numbers are all off by 1. I actually think the comments are unnecessary since the debugPrint is self-commenting.
| subscription: subscription, | ||
| ); | ||
| } catch (e) { | ||
| // Clean up on error |
There was a problem hiding this comment.
Should this call cleanupWebSocketTestResources?
| debugPrint('Step 5: Testing hot reload with WebSocket connection...'); | ||
| try { | ||
| // Test hot reload functionality | ||
| debugPrint('Step 6: Testing hot reload with WebSocket connection...'); |
There was a problem hiding this comment.
Having step numbers here requires keeping all these files in sync. If the common file changes then we would have to update this one too. I think for the actual testing we can omit the step numbers.
Closes dart-lang/webdev#2669
Blocked by hot restart bug: Fix in dart-lang/webdev#2668 and #173777