Skip to content

Start adding Sky support to test runner #320

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Start adding Sky support to test runner #320

wants to merge 4 commits into from

Conversation

abarth
Copy link
Contributor

@abarth abarth commented Aug 16, 2015

I'm working on adding support for Sky to test by making it possible to run tests in sky_shell. The sky_shell is similar to the standalone dart command line environment but with the dart:sky library available.

The approach I'm using is to run the tests in a sky_shell subprocess of the test runner process. The code will follow the approach used by TestPlatform.vm but using subprocesses and WebSockets instead of isolates and SendPorts.

I looked at re-using the browser codepath, but that code makes too many assumptions about how the subprocess works (e.g., that it supports iframes). It looked like TestPlatform.vm was a better starting point.

This pull request just factors base classes out of some of the runner/vm code to make it easier to share code with the upcoming subprocess_listener and subprocess_test. After these series of pull requests, it will also be possible to run tests in dart subprocesses instead of isolates.

abarth added 3 commits August 16, 2015 00:24
 - Improve BrowserServer.loadSuite return type
 - Move comment to proper location
VMListener will be used when running tests in
subprocesses instead of isolates.
This code will be shared with SubprocessTest to
support running tests in another process.
@abarth
Copy link
Contributor Author

abarth commented Aug 16, 2015

/cc @nex3 @sethladd

This patch moves the Isolate-related concerns out of the main `Loader` class
and into an `IsolateLoader` class. This change makes the structure between
Isolate- and Browser-based loading more parallel and paves the way to introduce
a `SubprocessLoader`.
@nex3
Copy link
Member

nex3 commented Aug 17, 2015

This is very cool stuff, and we definitely want to make it easy to run the test runner with Sky. But wouldn't it be a lot simpler and faster to just run pub with the sky_shell executable, so pub run test just works?

@abarth
Copy link
Contributor Author

abarth commented Aug 17, 2015

The sky_shell executable doesn't support all of the same features as the dart executable. For example, you can't create additional isolates.

@nex3
Copy link
Member

nex3 commented Aug 17, 2015

In that case, this would be a great place for a launcher plugin (#99).

How urgent is it for you to get the test runner running Sky? If possible I'd like to avoid a major refactor that would later be undone, but it'll be a while before plugin support lands and I don't want to block you. How are you thinking that Sky support would be injected into the test runner?

@abarth
Copy link
Contributor Author

abarth commented Aug 17, 2015

In that case, this would be a great place for a launcher plugin (#99).

I don't think the Browser API is a good fit for Sky. I started down that path, but essentially none of the infrastructure in the browser directory was useful. The browser directory assumes that you want a long-lived server that can run many tests in containers. That's appropriate for browsers because they are slow to load and have iframes, but Sky is fast to load and just runs one thing at a time, like the dart command line program.

Instead, the model in the vm directory fit well with what we want for Sky. The only trouble is that it re-uses the existing process using isolates. If instead it spun up subprocesses instead of isolates, it would be exactly what we want.

How urgent is it for you to get the test runner running Sky?

Currently we write tests using a different test runner written in Python. That works ok for us, but it's not something we'll want end-developers using Sky to have to deal with. We'd like to switch over to using the same testing system our end-developers will be using.

If possible I'd like to avoid a major refactor that would later be undone, but it'll be a while before plugin support lands and I don't want to block you.

I don't think the plugin system as described in #99 will work well for Sky. Sky isn't a browser, so having the ability to plug in different browsers isn't that useful.

How are you thinking that Sky support would be injected into the test runner?

Assuming we have multiprocess support in the vm code path, the only missing piece of information would be the path to the sky_shell binary to use when creating the subprocess. We could either pass that path in on the command line or create a TestPlatform subclass for Sky that knows where to find the binary.

@nex3
Copy link
Member

nex3 commented Aug 17, 2015

I don't think the Browser API is a good fit for Sky. I started down that path, but essentially none of the infrastructure in the browser directory was useful. The browser directory assumes that you want a long-lived server that can run many tests in containers. That's appropriate for browsers because they are slow to load and have iframes, but Sky is fast to load and just runs one thing at a time, like the dart command line program.

Instead, the model in the vm directory fit well with what we want for Sky. The only trouble is that it re-uses the existing process using isolates. If instead it spun up subprocesses instead of isolates, it would be exactly what we want.

Good point; maybe #49 is a better fit. The plugin API design is still up in the air, and we'll probably use this as a driving use-case, so it'll fit in one way or another.

Assuming we have multiprocess support in the vm code path, the only missing piece of information would be the path to the sky_shell binary to use when creating the subprocess. We could either pass that path in on the command line or create a TestPlatform subclass for Sky that knows where to find the binary.

Since it sounds like you've got a workable (if not ideal) setup for now, I think I'd like to keep this refactor out of the core test runner and work towards fitting it into the plugin API. Especially since this would be a user-visible change, rolling it back once the plugin API lands would be a pretty big cost.

@nex3 nex3 closed this Aug 17, 2015
nex3 added a commit that referenced this pull request Aug 21, 2015
This is specifically to allow Sky to get sky_shell support into the test
runner immediately. It's temporary; it'll last until we land proper
plugin support.

See #320

[email protected]

Review URL: https://codereview.chromium.org//1309493002 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants