[S] Change in dart/sdk[main]: [dart2wasm, infra] Add dart2wasm minify test configuration
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
@kuste...@google.com this adds a test configuration with `--minify`. We want to test with and without `--minify`, but I'm not sure how useful this test configuration is on its own as a lot of tests fail with it. Thoughts on how to best test `--minify`?
Related details
- Martin Kustermann
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Martin Kustermann
| Code-Review | +1 |
1 comment
@kuste...@google.com this adds a test configuration with `--minify`. We want to test with and without `--minify`, but I'm not sure how useful this test configuration is on its own as a lot of tests fail with it. Thoughts on how to best test `--minify`?
As mentioned before, individual tests can opt into `--minify` via `// dart2wasmOptions=...` mechanism.
It would be even better to test all tests in that mode, but naturally some tests will actually depend on `<obj>.runtimeType.toString()` / etc
So the way this should be handled is that the test itself has to be made aware of minification mode and it can skip the test or test something else in minified mode.
The mechanism we have for this is
```
import 'package:expect/variations.dart' show readableTypeStrings, ...;
main() {
if (!readableTypeStrings) return;
// code that relies on `<obj>.runtimeType.toString()`
}
```Some tests don't use this mechanism yet, so we'd need to update those tests (e.g. dart2wasm only tests, all tests that run under dart2js are probably already using this mechanism)
=> We have to update the `pkg/expect/lib/variations.dart` getters to work with dart2wasm. Currenlty they are e.g.
```
bool get readableTypeStrings
=> !const bool.fromEnvironment('dart.tool.dart2js.minify');
```
Then make the compiler define those environment variables.
But since this CL only adds the configuration and not the actual CI builder to run tests, we can land the CL as is.
A following CL can do the changes mentioned above and then we can add a CI builder (or change an existing one to run in minified mode)
Related details
- Ömer Ağacan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
Martin Kustermann@kuste...@google.com this adds a test configuration with `--minify`. We want to test with and without `--minify`, but I'm not sure how useful this test configuration is on its own as a lot of tests fail with it. Thoughts on how to best test `--minify`?
As mentioned before, individual tests can opt into `--minify` via `// dart2wasmOptions=...` mechanism.
It would be even better to test all tests in that mode, but naturally some tests will actually depend on `<obj>.runtimeType.toString()` / etc
So the way this should be handled is that the test itself has to be made aware of minification mode and it can skip the test or test something else in minified mode.
The mechanism we have for this is
```
import 'package:expect/variations.dart' show readableTypeStrings, ...;main() {
if (!readableTypeStrings) return;
// code that relies on `<obj>.runtimeType.toString()`
}
```Some tests don't use this mechanism yet, so we'd need to update those tests (e.g. dart2wasm only tests, all tests that run under dart2js are probably already using this mechanism)
=> We have to update the `pkg/expect/lib/variations.dart` getters to work with dart2wasm. Currenlty they are e.g.
```
bool get readableTypeStrings
=> !const bool.fromEnvironment('dart.tool.dart2js.minify');
```Then make the compiler define those environment variables.
But since this CL only adds the configuration and not the actual CI builder to run tests, we can land the CL as is.
A following CL can do the changes mentioned above and then we can add a CI builder (or change an existing one to run in minified mode)
Instead of `variations` we could also use status files to skip tests when minimized. `TestConfiguration` already has a field for it: https://github.com/dart-lang/sdk/blob/6c81b537c08b540b11d3b82c7ee202b774e5d6c9/pkg/test_runner/lib/src/configuration.dart#L121 we could expose it as a variable in status files.
It's confusing that some of the compile-time features are in test configuration and can be checked in status files, while others are checked in compile-time via the `variations` library. If I want to add another test configuration check like minification how do I decide which one to add the check for it?
Related details
- Martin Kustermann
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Kustermann (Gerrit)
Martin Kustermann added 1 comment![]( "Open in Gerrit")
Martin Kustermann@kuste...@google.com this adds a test configuration with `--minify`. We want to test with and without `--minify`, but I'm not sure how useful this test configuration is on its own as a lot of tests fail with it. Thoughts on how to best test `--minify`?
Ömer AğacanAs mentioned before, individual tests can opt into `--minify` via `// dart2wasmOptions=...` mechanism.
It would be even better to test all tests in that mode, but naturally some tests will actually depend on `<obj>.runtimeType.toString()` / etc
So the way this should be handled is that the test itself has to be made aware of minification mode and it can skip the test or test something else in minified mode.
The mechanism we have for this is
```
import 'package:expect/variations.dart' show readableTypeStrings, ...;main() {
if (!readableTypeStrings) return;
// code that relies on `<obj>.runtimeType.toString()`
}
```Some tests don't use this mechanism yet, so we'd need to update those tests (e.g. dart2wasm only tests, all tests that run under dart2js are probably already using this mechanism)
=> We have to update the `pkg/expect/lib/variations.dart` getters to work with dart2wasm. Currenlty they are e.g.
```
bool get readableTypeStrings
=> !const bool.fromEnvironment('dart.tool.dart2js.minify');
```Then make the compiler define those environment variables.
But since this CL only adds the configuration and not the actual CI builder to run tests, we can land the CL as is.
A following CL can do the changes mentioned above and then we can add a CI builder (or change an existing one to run in minified mode)
Instead of `variations` we could also use status files to skip tests when minimized. `TestConfiguration` already has a field for it: https://github.com/dart-lang/sdk/blob/6c81b537c08b540b11d3b82c7ee202b774e5d6c9/pkg/test_runner/lib/src/configuration.dart#L121 we could expose it as a variable in status files.
It's confusing that some of the compile-time features are in test configuration and can be checked in status files, while others are checked in compile-time via the `variations` library. If I want to add another test configuration check like minification how do I decide which one to add the check for it?
Yes, theoretically one could use status files for it and yes there's already a `$minified` boolean (which may mean different things depending on backend). For some tests where the entire test depends on symbols maybe that makes sense to use status files.
Though often a test tests many things and only part of it is relying on symbols, in which case the `variations` approach works better as one can selectively disable one of the things a test is testing instead of the the entire test.
Generally I'd say we prefer `variations` over the status files.
Related details
- Ömer Ağacan
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan voted Commit-Queue+2![]( "Open in Gerrit")
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
[dart2wasm, infra] Add dart2wasm minify test configuration
- M tools/bots/test_matrix.json
Code-Review: +1 by Martin Kustermann
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |