Skip to content

Conversation

nex3
Copy link
Member

@nex3 nex3 commented Oct 3, 2017

We want users to be able to dynamically define new platforms, which
means we need infrastructure in place for piping those platforms to
places that previously assumed TestPlatform.all was a full list of
available platforms. PlatformSelector is the trickiest example, since
it's parsed in a number of different places and needs to provide useful
feedback to users when they use an undefined platform.

This splits parsing and platform validation into two separate steps.
Validation will be done immediately after parsing when the selectors
come from top-level annotations or parameters passed to test() or
group(), but selectors defined in configuration files are now parsed
only after all configuration is parsed. This will allow new platforms to
be defined and referenced in configuration files.

See #99
See #391

We want users to be able to dynamically define new platforms, which
means we need infrastructure in place for piping those platforms to
places that previously assumed TestPlatform.all was a full list of
available platforms. PlatformSelector is the trickiest example, since
it's parsed in a number of different places and needs to provide useful
feedback to users when they use an undefined platform.

This splits parsing and platform validation into two separate steps.
Validation will be done immediately after parsing when the selectors
come from top-level annotations or parameters passed to test() or
group(), but selectors defined in configuration files are now parsed
only after all configuration is parsed. This will allow new platforms to
be defined *and* referenced in configuration files.

See #99
See #391
@nex3 nex3 requested a review from grouma October 3, 2017 20:51
Copy link
Member

@grouma grouma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have an example of how one would define a new platform?

var parser = new ArgParser(allowTrailingOptions: true);

var allPlatforms = TestPlatform.all.toList();
var allPlatforms = TestPlatform.all.toList()..remove(TestPlatform.vm);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we remove the vm platform here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we manually include it in the list so we can add "(default)" after it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see.

@nex3
Copy link
Member Author

nex3 commented Oct 3, 2017

Do you have an example of how one would define a new platform?

Actually defining a platform isn't supported yet; this is just laying the groundwork. But my plan is laid out in #391.

Copy link
Member

@grouma grouma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. We'll have to be careful when we pull this into Google3. As long as this: https://github.com/dart-lang/test/blob/master/lib/src/backend/test_platform.dart#L111 still works we should be good.

var parser = new ArgParser(allowTrailingOptions: true);

var allPlatforms = TestPlatform.all.toList();
var allPlatforms = TestPlatform.all.toList()..remove(TestPlatform.vm);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see.

@nex3
Copy link
Member Author

nex3 commented Oct 3, 2017

Noted. We'll have to be careful when we pull this into Google3. As long as this: https://github.com/dart-lang/test/blob/master/lib/src/backend/test_platform.dart#L111 still works we should be good.

As this CL stands, that should continue to work, but I'm hoping to refactor it a bit to make it more similar to other plugin API stuff we expose.

@nex3 nex3 merged commit 7f7d218 into feature.flexible-test-platforms Oct 3, 2017
@nex3 nex3 deleted the deglobal-platform-selector branch October 3, 2017 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants