Replace the configuration map with a typed object.

Replace the configuration map with a typed object.

This is a pretty massive change but my hope is it will make
test.dart easier to maintain going forward. Instead of
passing around a stringly-typed Map<String, dynamic> all
throughout the program, we parse the options and then create
a Configuration object that has typed getters for all of the
various bits of configuration data.

This is a little tedious because it means declaring a new
option requires also declaring a corresponding field in the
Configuration class and passing it through the constructor.
I think it's worth it.

Also, enum-like configuration properties like architecture
and runtime now have their own classes as well. Moved a
bunch of stuff from TestUtil into those classes now that
there is an object to hang those methods off of.

In the process, I found a few typos in string literals where
the code wasn't correctly looking up a configuration
property.

Added more sanity checking and validation to status file
parsing. You will get an error at parse time if you try to
refer to a variable that isn't in the whitelist of known
variables. Also, you'll get an error if you try to compare
a variable to a value that it isn't expected to have.

Many other small-scale cleanups.

Aside from the status file validation, this should behave
mostly the same as current test.dart except that tests
may be enqueued in a slightly different order. The
rewritten code for expanding configurations iterates
through the architecture, runtime, etc. options in a
slightly different order.

R=whesse@google.com

Committed: https://github.com/dart-lang/sdk/commit/06f75fe5cd99aa5f85d2afaacd6dddc95e1570d9

[Bob Nystrom, 2017-05-24 01:14:57]

rnystrom@google.com changed reviewers:
+ whesse@google.com

[Bob Nystrom, 2017-05-24 01:14:58]

I know this patch is a monster. I'm sorry. :(

This is the last big chunk of refactoring I'm planning to do on test.dart for
now. If you'd like, I can try to split some off into easier to review smaller
pieces, but that's pretty difficult. As you can imagine, replacing the
Map<String, dynamic> that was used everywhere for configuration with a real
typed object is a pretty pervasive change.

If you can survive reviewing this, I promise to do less sweeping changes
afterwards.

Thanks!

[Bill Hesse, 2017-05-29 13:08:28]

LGTM.

I think record_to_file and replay_from_file are now unused, but they can be
removed in a separate CL.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/browser_...
File tools/testing/dart/browser_controller.dart (left):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/browser_...
tools/testing/dart/browser_controller.dart:1005: browserName = (browserName ==
'ff') ? 'firefox' : browserName,
I'm very happy about removing this line.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/browser_...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/browser_...
tools/testing/dart/browser_controller.dart:1209: if (configuration.runtime ==
Runtime.ie10 ||
Need parentheses around the || expression: (a || b) && c

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/configur...
File tools/testing/dart/configuration.dart (right):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:216: // args.add("--verbose");
Remove commented code.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:253: /// Gets the local file path to the
browser executable for this configuration.
I think this would go better in runtime_configuration

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:327: RuntimeConfiguration
_runtimeConfiguration;
Same here - couldn't the function go in runtime_configuration, and only the
getter and cache be in the configuration?

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:647: switch (this) {
Could these be on the compiler configurations, or aren't they created at the
point we need these?

Could they move to compiler_configuration in any case?

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/environm...
File tools/testing/dart/environment.dart (right):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/environm...
tools/testing/dart/environment.dart:10: // TODO(rnystrom): Instead of
synthesized negated variables like "unchecked",
File issue for TODO, use issue number

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/options....
File tools/testing/dart/options.dart (right):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:484: var selectors =
_expandSelectors(configuration);
Because our code review and code browsing tools don't provide easy access to
type inference, the rule about leaving local variables untyped makes it harder
to read and review this code, so I would declare the local variable as a list of
String.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:495: // observatory_ui, and remove it from the
original selectors.
and remove "observatory_ui" from the original selectors list.

("it" otherwise seems to mean the new configuration, on first read)

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:510: // Return the expansions of both
configurations. Neither will reach
Remove this comment, the call is no longer recursive (thanks).

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/runtime_...
File tools/testing/dart/runtime_configuration.dart (left):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/runtime_...
tools/testing/dart/runtime_configuration.dart:25: // TODO(ahe): Remove this
constructor and move the switch to
I agree this could be made a static method, not a constructor, but I don't see
the advantage to moving it to test_options.dart.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/test_con...
File tools/testing/dart/test_configurations.dart (right):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/test_con...
tools/testing/dart/test_configurations.dart:112: if (configuration.isStrong)
settings.add('strong');
Please add printing of "fast-startup" if fast startup here, if you don't mind
adding something.

[Bob Nystrom, 2017-05-30 23:29:31]

Thanks for the review!

It's a little late in the afternoon to land a CL this monumental, so I'll wait
until tomorrow. Let me know if there are any other changes you'd like me to
make.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/browser_...
File tools/testing/dart/browser_controller.dart (left):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/browser_...
tools/testing/dart/browser_controller.dart:1005: browserName = (browserName ==
'ff') ? 'firefox' : browserName,
On 2017/05/29 13:08:27, Bill Hesse wrote:
> I'm very happy about removing this line.

Alas, its legacy lives on to some degree in a few other places. I'd love to
consolidate onto a single canonical name, but the status files seem to prefer
"ff" but "firefox" seems like the more readable name to me. :-/

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/browser_...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/browser_...
tools/testing/dart/browser_controller.dart:1209: if (configuration.runtime ==
Runtime.ie10 ||
On 2017/05/29 13:08:27, Bill Hesse wrote:
> Need parentheses around the || expression: (a || b) && c

Oh, dang. Nice catch. Done.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/configur...
File tools/testing/dart/configuration.dart (right):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:216: // args.add("--verbose");
On 2017/05/29 13:08:28, Bill Hesse wrote:
> Remove commented code.

Done.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:253: /// Gets the local file path to the
browser executable for this configuration.
On 2017/05/29 13:08:28, Bill Hesse wrote:
> I think this would go better in runtime_configuration

This relies on chromePath, dartiumPath, et al., which are all defined on
Configuration. I could move this into RuntimeConfiguration (or Runtime for that
matter), but then those would all have to be passed in, so I don't think it's
worth doing. If it didn't use those, definitely I would move it down.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:327: RuntimeConfiguration
_runtimeConfiguration;
On 2017/05/29 13:08:28, Bill Hesse wrote:
> Same here - couldn't the function go in runtime_configuration, and only the
> getter and cache be in the configuration?

My goal was to get rid of all of the "configuration." calls in the body,
especially in CompilerConfiguration. It seemed a little shorter to move the
logic right into Configuration. But I agree, it's weird. Moved it back.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/configur...
tools/testing/dart/configuration.dart:647: switch (this) {
On 2017/05/29 13:08:28, Bill Hesse wrote:
> Could these be on the compiler configurations, or aren't they created at the
> point we need these?
> 
> Could they move to compiler_configuration in any case?

They could, yes. But they don't rely on any of the state in
CompilerConfiguration, so I thought it made more sense to have them here. My
rough rule of thumb is to put code near the data it requires and put methods on
the most stateless classes there are that still have the data the method needs.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/environm...
File tools/testing/dart/environment.dart (right):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/environm...
tools/testing/dart/environment.dart:10: // TODO(rnystrom): Instead of
synthesized negated variables like "unchecked",
On 2017/05/29 13:08:28, Bill Hesse wrote:
> File issue for TODO, use issue number

Done.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/options....
File tools/testing/dart/options.dart (right):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:484: var selectors =
_expandSelectors(configuration);
On 2017/05/29 13:08:28, Bill Hesse wrote:
> Because our code review and code browsing tools don't provide easy access to
> type inference, the rule about leaving local variables untyped makes it harder
> to read and review this code, so I would declare the local variable as a list
of
> String.

Yeah, I know it can make reviews a little harder sometimes. I deal with this too
since I review a lot of packages that don't use type annotations for locals.
I've found I'm used to it enough that it doesn't bother me now.

If it's OK with you, I'll keep it inferred. With a style rule like "use
inference if the type is obvious", it gets really murky about what "obvious"
means, especially as code evolves over time. So I tend to prefer a simpler rule
which is to use type inference for all locals where inference produces the right
result. There's less to think about that way.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:495: // observatory_ui, and remove it from the
original selectors.
On 2017/05/29 13:08:28, Bill Hesse wrote:
> and remove "observatory_ui" from the original selectors list.
> 
> ("it" otherwise seems to mean the new configuration, on first read)

Good point. Done.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/options....
tools/testing/dart/options.dart:510: // Return the expansions of both
configurations. Neither will reach
On 2017/05/29 13:08:28, Bill Hesse wrote:
> Remove this comment, the call is no longer recursive (thanks).

Done.

It took me quite a while to wrap my head around what the original code was
doing! :)

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/runtime_...
File tools/testing/dart/runtime_configuration.dart (left):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/runtime_...
tools/testing/dart/runtime_configuration.dart:25: // TODO(ahe): Remove this
constructor and move the switch to
On 2017/05/29 13:08:28, Bill Hesse wrote:
> I agree this could be made a static method, not a constructor, but I don't see
> the advantage to moving it to test_options.dart.

Moved it back.

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/test_con...
File tools/testing/dart/test_configurations.dart (right):

https://codereview.chromium.org/2901923003/diff/1/tools/testing/dart/test_con...
tools/testing/dart/test_configurations.dart:112: if (configuration.isStrong)
settings.add('strong');
On 2017/05/29 13:08:28, Bill Hesse wrote:
> Please add printing of "fast-startup" if fast startup here, if you don't mind
> adding something.

Done.

[Bob Nystrom, 2017-05-31 21:47:35]

Description was changed from

==========
Replace the configuration map with a typed object.

This is a pretty massive change but my hope is it will make
test.dart easier to maintain going forward. Instead of
passing around a stringly-typed Map<String, dynamic> all
throughout the program, we parse the options and then create
a Configuration object that has typed getters for all of the
various bits of configuration data.

This is a little tedious because it means declaring a new
option requires also declaring a corresponding field in the
Configuration class and passing it through the constructor.
I think it's worth it.

Also, enum-like configuration properties like architecture
and runtime now have their own classes as well. Moved a
bunch of stuff from TestUtil into those classes now that
there is an object to hang those methods off of.

In the process, I found a few typos in string literals where
the code wasn't correctly looking up a configuration
property.

Added more sanity checking and validation to status file
parsing. You will get an error at parse time if you try to
refer to a variable that isn't in the whitelist of known
variables. Also, you'll get an error if you try to compare
a variable to a value that it isn't expected to have.

Many other small-scale cleanups.

Aside from the status file validation, this should behave
mostly the same as current test.dart except that tests
may be enqueued in a slightly different order. The
rewritten code for expanding configurations iterates
through the architecture, runtime, etc. options in a
slightly different order.
==========

to

==========
Replace the configuration map with a typed object.

This is a pretty massive change but my hope is it will make
test.dart easier to maintain going forward. Instead of
passing around a stringly-typed Map<String, dynamic> all
throughout the program, we parse the options and then create
a Configuration object that has typed getters for all of the
various bits of configuration data.

This is a little tedious because it means declaring a new
option requires also declaring a corresponding field in the
Configuration class and passing it through the constructor.
I think it's worth it.

Also, enum-like configuration properties like architecture
and runtime now have their own classes as well. Moved a
bunch of stuff from TestUtil into those classes now that
there is an object to hang those methods off of.

In the process, I found a few typos in string literals where
the code wasn't correctly looking up a configuration
property.

Added more sanity checking and validation to status file
parsing. You will get an error at parse time if you try to
refer to a variable that isn't in the whitelist of known
variables. Also, you'll get an error if you try to compare
a variable to a value that it isn't expected to have.

Many other small-scale cleanups.

Aside from the status file validation, this should behave
mostly the same as current test.dart except that tests
may be enqueued in a slightly different order. The
rewritten code for expanding configurations iterates
through the architecture, runtime, etc. options in a
slightly different order.

R=whesse@google.com

Committed:
https://github.com/dart-lang/sdk/commit/06f75fe5cd99aa5f85d2afaacd6dddc95e1570d9
==========

[Bob Nystrom, 2017-05-31 21:47:36]

Committed patchset #3 (id:40001) manually as
06f75fe5cd99aa5f85d2afaacd6dddc95e1570d9 (presubmit successful).