Migrate test/fixtures/context.dart to null-safety
#1698
Conversation
dwds/test/fixtures/context.dart
Outdated
| CompilationMode compilationMode = CompilationMode.buildDaemon, | ||
| NullSafety nullSafety = NullSafety.weak, | ||
| NullSafety? nullSafety = NullSafety.weak, |
There was a problem hiding this comment.
Can this be non-nullable with the default value of NullSafety.weak? I don't think we have a use for a null value in this case.
dwds/test/fixtures/context.dart
Outdated
| ResidentWebRunner get webRunner => _webRunner!; | ||
| late ResidentWebRunner? _webRunner; | ||
|
|
||
| // here |
dwds/test/fixtures/context.dart
Outdated
| late BuildDaemonClient? _daemonClient; | ||
|
|
||
| ResidentWebRunner get webRunner => _webRunner!; | ||
| late ResidentWebRunner? _webRunner; |
There was a problem hiding this comment.
We don't always have a web runner (only in frontend server mode), should this not be late?
dwds/test/fixtures/context.dart
Outdated
| late Client? _client; | ||
|
|
||
| ExpressionCompilerService get ddcService => _ddcService!; | ||
| late ExpressionCompilerService? _ddcService; |
There was a problem hiding this comment.
We don't always have a ddc service (only in build runner mode with expression evaluation enabled), should this not be late?
dwds/test/fixtures/context.dart
Outdated
| @@ -90,11 +113,11 @@ class TestContext { | |||
| /// TODO(annagrin): Currently setting sound null safety for frontend | |||
| /// server tests fails due to missing sound SDK JavaScript and maps. | |||
| /// Issue: https://github.com/dart-lang/webdev/issues/1591 | |||
| NullSafety nullSafety; | |||
| NullSafety? nullSafety; | |||
There was a problem hiding this comment.
I think this could be final non-nullable, with a default value of NullSafety.weak
There was a problem hiding this comment.
Looks like this isn't used, therefore removed
dwds/test/fixtures/context.dart
Outdated
| }) async { | ||
| sdkConfigurationProvider ??= DefaultSdkConfigurationProvider(); | ||
| urlEncoder = urlEncoder ?? (url) async => url; |
There was a problem hiding this comment.
I thought we allow a null url encoder, does it fail? Maybe some of the null safety migration changes was too strict? Since we are testing all scenarios here, we need to either require the url encoder to be non-null everywhere or allow a null here.
There was a problem hiding this comment.
The problem is the ResidentWebRunner requires a non-null UrlEncoder: https://github.com/dart-lang/webdev/blob/master/dwds/test/fixtures/context.dart#L268. For the frontend_server_???_tests, we aren't passing in a URL encoder so without this they fail. I could also explicitly pass in a url encoder from these tests. Wdyt?
There was a problem hiding this comment.
Discussed offline, ResidentWebRunner should accept a null urlEncoder
| @@ -402,7 +409,7 @@ class TestContext { | |||
| _entryContents.replaceAll('Hello World!', 'Gary is awesome!')); | |||
|
|
|||
| // Wait for the build. | |||
| await daemonClient.buildResults.firstWhere((results) => results.results | |||
| await _daemonClient?.buildResults.firstWhere((results) => results.results | |||
There was a problem hiding this comment.
Can _daemonClient be null here? If it could, I'd presume the tests might not work as there are no build results, so we probably should fail in that case anyway.
Btw, I see that _daemonClient is treated differently than the other fields of TestContext (ie there is no getter that asserts that _daemonClient is non-null) - is there a reason for the difference?
| TestServer get testServer => _testServer!; | ||
| TestServer? _testServer; | ||
|
|
||
| BuildDaemonClient? _daemonClient; |
There was a problem hiding this comment.
Would it be possible to make _daemonClient logic similar to other fields here?
No description provided.