Turn on Polymer TodoMVC tests on Dart buildbots

Turn on Polymer TodoMVC tests on Dart buildbots

The biggest changes are to the examples, but there's a small tweak to test_suite.dart as well, so it can load the HTML files.

For now, we only test the development loop (content_shell+Dart VM). I'm working on changes to enable us to test the deploy story with dart2js.

R=kustermann@google.com, sigmund@google.com

Committed: https://code.google.com/p/dart/source/detail?r=26943

[Jennifer Messerly, 2013-08-28 04:46:03]

Hi Martin,

Do you mind having a look at the test_suite.dart changes?

Siggi, would you mind looking at the TodoMVC+Polymer changes?

Thanks!
-John

https://codereview.chromium.org/23539003/diff/3001/samples/tests/samples/samp...
File samples/tests/samples/samples.status (right):

https://codereview.chromium.org/23539003/diff/3001/samples/tests/samples/samp...
samples/tests/samples/samples.status:44: # TODO(jmesserly): this test needs to
be compiled with Polymer deploy step
@Siggi -- I actually got pretty close, then I hit a strange problem with getting
this working...

If I try to check in our generated deployment files (everything except the JS
compilation of course), DWC generates a directory structure that mirrors the
original directory structure. This means that in the status file, it becomes
impossible to refer to just the non-generated files. for example,
samples/todomvc/listorder will select itself *and*
samples/todomvc/listorder/generated/tests/samples/todomvc/listorder

... it's madness!

if DWC could rename the files somehow when compiling, that would help, but I
dunno how crazy that is. Or we can wait for "pub serve" to come along...

[Siggi Cherem (dart-lang), 2013-08-28 17:42:54]

lgtm!

https://codereview.chromium.org/23539003/diff/3001/samples/tests/samples/samp...
File samples/tests/samples/samples.status (right):

https://codereview.chromium.org/23539003/diff/3001/samples/tests/samples/samp...
samples/tests/samples/samples.status:44: # TODO(jmesserly): this test needs to
be compiled with Polymer deploy step
On 2013/08/28 04:46:03, John Messerly wrote:
> @Siggi -- I actually got pretty close, then I hit a strange problem with
getting
> this working...
> 
> If I try to check in our generated deployment files (everything except the JS
> compilation of course), DWC generates a directory structure that mirrors the
> original directory structure. This means that in the status file, it becomes
> impossible to refer to just the non-generated files. for example,
> samples/todomvc/listorder will select itself *and*
> samples/todomvc/listorder/generated/tests/samples/todomvc/listorder
> 
> ... it's madness!
> 
> if DWC could rename the files somehow when compiling, that would help, but I
> dunno how crazy that is. Or we can wait for "pub serve" to come along...

mmm. I'm not sure I understand all the details, but let's talk more about this
in person

https://codereview.chromium.org/23539003/diff/6001/samples/tests/samples/todo...
File samples/tests/samples/todomvc/listorder_test.dart (right):

https://codereview.chromium.org/23539003/diff/6001/samples/tests/samples/todo...
samples/tests/samples/todomvc/listorder_test.dart:34: // fix it
programmatically.
FYI - I noticed this was broken also in the transformer code. My CL doing the
deploy step has a fix for the transformer code, but we still need to fix this in
boot.js

[kustermann, 2013-08-28 17:56:45]

I'll take a closer look tomorrow.

https://codereview.chromium.org/23539003/diff/6001/pkg/polymer/lib/testing/te...
File pkg/polymer/lib/testing/test_controller.js (left):

https://codereview.chromium.org/23539003/diff/6001/pkg/polymer/lib/testing/te...
pkg/polymer/lib/testing/test_controller.js:28: }
Why did you remove this?

This is needed for dartium/content_shell.

https://codereview.chromium.org/23539003/diff/6001/pkg/polymer/lib/testing/te...
File pkg/polymer/lib/testing/test_controller.js (right):

https://codereview.chromium.org/23539003/diff/6001/pkg/polymer/lib/testing/te...
pkg/polymer/lib/testing/test_controller.js:10: * It has been modified to work
with package:polymer/boot.js.
This is really bad. We should not fork!
It's really error prone if we have to keep these two in sync.

What is the difference between this and the original version?
Could extend the original version to make it work for polymer as well?

https://codereview.chromium.org/23539003/diff/6001/samples/tests/samples/todo...
File samples/tests/samples/todomvc/listorder_test.html (right):

https://codereview.chromium.org/23539003/diff/6001/samples/tests/samples/todo...
samples/tests/samples/todomvc/listorder_test.html:14: <script
src="packages/polymer/testing/test_controller.js"></script>
Does this mean we've 'packages' directories inside a checkout?
At which point do we call 'pub update'? (our testing scripts won't do it)

[Jennifer Messerly, 2013-08-28 18:11:05]

https://codereview.chromium.org/23539003/diff/6001/pkg/polymer/lib/testing/te...
File pkg/polymer/lib/testing/test_controller.js (left):

https://codereview.chromium.org/23539003/diff/6001/pkg/polymer/lib/testing/te...
pkg/polymer/lib/testing/test_controller.js:28: }
On 2013/08/28 17:56:45, kustermann wrote:
> Why did you remove this?
> 
> This is needed for dartium/content_shell.

Polymer's "boot.js" knows how to start Dartium. That's the only reason to fork
this file. test_controller.js is trying to start Dartium way too soon for
boot.js to work properly.

https://codereview.chromium.org/23539003/diff/6001/pkg/polymer/lib/testing/te...
File pkg/polymer/lib/testing/test_controller.js (right):

https://codereview.chromium.org/23539003/diff/6001/pkg/polymer/lib/testing/te...
pkg/polymer/lib/testing/test_controller.js:10: * It has been modified to work
with package:polymer/boot.js.
On 2013/08/28 17:56:45, kustermann wrote:
> This is really bad. We should not fork!
> It's really error prone if we have to keep these two in sync.

agreed, we already have a copy in polymer/lib/testing.js though, so at least it
isn't making it worse. :)

I'm trying to get browser test coverage for Polymer again (currently it is at 0
browser tests running) hence a bit of desperation on my part to get something
working and iterate...

> 
> What is the difference between this and the original version?
> Could extend the original version to make it work for polymer as well?

The difference is below. We can't call webkitStartDart in this file, it needs to
happen later when Polymer's boot.js is ready.

here's an idea: I could add a flag to unittest/lib/test_controller.js to
suppress the webkitStartDart call, and set that from polymer/lib/boot.js. Would
that work for you?

FYI here is boot.js:
https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/polymer/lib/b...

[Jennifer Messerly, 2013-08-28 18:13:09]

https://codereview.chromium.org/23539003/diff/6001/samples/tests/samples/todo...
File samples/tests/samples/todomvc/listorder_test.html (right):

https://codereview.chromium.org/23539003/diff/6001/samples/tests/samples/todo...
samples/tests/samples/todomvc/listorder_test.html:14: <script
src="packages/polymer/testing/test_controller.js"></script>
On 2013/08/28 17:56:45, kustermann wrote:
> Does this mean we've 'packages' directories inside a checkout?
> At which point do we call 'pub update'? (our testing scripts won't do it)

We don't need to, the test server handles "packages" in a smart way. I was quite
pleasantly surprised to discover this. :)

https://github.com/dart-lang/bleeding_edge/blob/master/dart/tools/testing/dar...

[Siggi Cherem (dart-lang), 2013-08-28 18:21:10]

https://codereview.chromium.org/23539003/diff/6001/pkg/polymer/lib/testing/te...
File pkg/polymer/lib/testing/test_controller.js (right):

https://codereview.chromium.org/23539003/diff/6001/pkg/polymer/lib/testing/te...
pkg/polymer/lib/testing/test_controller.js:10: * It has been modified to work
with package:polymer/boot.js.
On 2013/08/28 18:11:06, John Messerly wrote:
> On 2013/08/28 17:56:45, kustermann wrote:
> > This is really bad. We should not fork!
> > It's really error prone if we have to keep these two in sync.
> 
> agreed, we already have a copy in polymer/lib/testing.js though, so at least
it
> isn't making it worse. :)
> 
> I'm trying to get browser test coverage for Polymer again (currently it is at
0
> browser tests running) hence a bit of desperation on my part to get something
> working and iterate...
> 
> > 
> > What is the difference between this and the original version?
> > Could extend the original version to make it work for polymer as well?
> 
> The difference is below. We can't call webkitStartDart in this file, it needs
to
> happen later when Polymer's boot.js is ready.
> 
> here's an idea: I could add a flag to unittest/lib/test_controller.js to
> suppress the webkitStartDart call, and set that from polymer/lib/boot.js.
Would
> that work for you?
> 
> FYI here is boot.js:
>
https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/polymer/lib/b...

I was also thinking of the same approach =)

[Jennifer Messerly, 2013-08-28 19:19:31]

Thanks guys, PTAL!

@Martin -- it now uses unittest's test_controller.js. Small change there to skip
webkitStartDart if HTMLImports package is being used.

@Siggi -- please take a look at observe/lib/microtask.js. This change is
required for tests to pass, I had accidentally reverted it when I was merging.
The key aspect of this change is to have performMicrotaskCheckpoint work
correctly if called from inside another performMicrotaskCheckpoint.

[Siggi Cherem (dart-lang), 2013-08-28 19:46:51]

https://codereview.chromium.org/23539003/diff/16001/pkg/observe/lib/src/micro...
File pkg/observe/lib/src/microtask.dart (right):

https://codereview.chromium.org/23539003/diff/16001/pkg/observe/lib/src/micro...
pkg/observe/lib/src/microtask.dart:33: final startLength = _pending.length;
delete (unused variable)

https://codereview.chromium.org/23539003/diff/16001/pkg/unittest/lib/test_con...
File pkg/unittest/lib/test_controller.js (right):

https://codereview.chromium.org/23539003/diff/16001/pkg/unittest/lib/test_con...
pkg/unittest/lib/test_controller.js:27: if (navigator.webkitStartDart &&
!window.HTMLImports) {
mmm. I worry there might be cases where people use htmlimports without boot.js
and this script would not behave as expected. I wonder if we should just use a
generic flag and configure it from boot.js or the html test page. For example,
"window.__testControllerDontStartDart"?

[Jennifer Messerly, 2013-08-28 19:56:04]

https://codereview.chromium.org/23539003/diff/16001/pkg/observe/lib/src/micro...
File pkg/observe/lib/src/microtask.dart (right):

https://codereview.chromium.org/23539003/diff/16001/pkg/observe/lib/src/micro...
pkg/observe/lib/src/microtask.dart:33: final startLength = _pending.length;
On 2013/08/28 19:46:51, Siggi Cherem (dart-lang) wrote:
> delete (unused variable)

Done.

https://codereview.chromium.org/23539003/diff/16001/pkg/unittest/lib/test_con...
File pkg/unittest/lib/test_controller.js (right):

https://codereview.chromium.org/23539003/diff/16001/pkg/unittest/lib/test_con...
pkg/unittest/lib/test_controller.js:27: if (navigator.webkitStartDart &&
!window.HTMLImports) {
On 2013/08/28 19:46:51, Siggi Cherem (dart-lang) wrote:
> mmm. I worry there might be cases where people use htmlimports without boot.js
> and this script would not behave as expected. I wonder if we should just use a
> generic flag and configure it from boot.js or the html test page. For example,
> "window.__testControllerDontStartDart"?
> 

Chatted in person ... we should really move some of that the logic from
Polymer's boot.js to HTML Imports polyfill, so HTML Imports is the right thing
to check.

[Siggi Cherem (dart-lang), 2013-08-28 21:13:59]

lgtm

[kustermann, 2013-08-30 15:34:42]

lgtm

Here are a few notes, would be nice if you could comment on them (if
applicable):

As far as I know polymer is living on github. We don't have any polymer related
things in the dart repository (so far). I guess it is also tested somewhere else
(e.g. drone.io)?

Is the reason for this CL, that the editor includes polymer examples, and we
need to make sure they run? Or what is the reason?

If so, we *need* to test if things work also if we dart2js compile these
examples and run them in normal browsers. Doing that requires that we put the
compiled output somewhere (and a generated html file), which could make the
changes in 'test_suite.dart' somewhat obsolete (i.e. in that case we need a
different mechanism)?

I saw now that terry@ added a third_party/polymer/ShadowDOM dependency pointing
to an external github repository.
Please CC ricow@+kustermann@ on such changes. It's important that we know about
these things. We want to minimize our external dependencies as much as we can --
usually we have internal mirrors of repositories.
Now all our builders are fetching code from github. Could easily be the case
that github blocks us for whatever reason (e.g. it thinks we're DDos-ing it ...
[has happened before with googlecode]) -- and this would make all builders red!
[ricow added https://code.google.com/p/dart/wiki/ThirdPartyAndBinaryData].

https://codereview.chromium.org/23539003/diff/23001/pkg/unittest/lib/test_con...
File pkg/unittest/lib/test_controller.js (right):

https://codereview.chromium.org/23539003/diff/23001/pkg/unittest/lib/test_con...
pkg/unittest/lib/test_controller.js:27: if (navigator.webkitStartDart &&
!window.HTMLImports) {
Please elaborate here a bit (in particular mention the interaction with polymer)
-- as time goes by these kind of things can easily be forgotten.

https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/sam...
File samples/tests/samples/samples.status (right):

https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/sam...
samples/tests/samples/samples.status:46: todomvc/*: Skip
If you've Skip here, please file a bug and add the issue number. That's the
normal procedure we use.

https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/tod...
File samples/tests/samples/todomvc/listorder_test.html (right):

https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/tod...
samples/tests/samples/todomvc/listorder_test.html:12: <link rel="stylesheet"
href="../../../third_party/todomvc/web/base.css">
What is this 'third_party/todomvc'

I cannot remember that I've seen additions to third_party.
Is that from github?

[Siggi Cherem (dart-lang), 2013-08-30 16:34:31]

Hi Martin,

Good questions. I wrote a couple answers to some of your questions below

On 2013/08/30 15:34:42, kustermann wrote:
> lgtm
> 
> Here are a few notes, would be nice if you could comment on them (if
> applicable):
> 
> As far as I know polymer is living on github. We don't have any polymer
related
> things in the dart repository (so far). I guess it is also tested somewhere
else
> (e.g. drone.io)?

We moved polymer and all its dependencies to the repo in the last couple weeks.
You can find the sources under pkg/polymer/ (among other packages)

> 
> Is the reason for this CL, that the editor includes polymer examples, and we
> need to make sure they run? Or what is the reason?

The reason is actually so we can have better testing of polymer in our bots.
When we were in github we had our own custom way of running tests using a shell
script that internally used content_shell, this script was run by drone.io. When
we moved the code we realized that we needed to do a lot of work to integrate
our tests, but we decided to do this in two steps. Our non-browser tests are
currently run in the bots, but browser tests are currently run manually using
the old script. With this CL (and another one in progress) we'll be able to run
all polymer tests in the bots.

> 
> If so, we *need* to test if things work also if we dart2js compile these
> examples and run them in normal browsers. Doing that requires that we put the
> compiled output somewhere (and a generated html file), which could make the
> changes in 'test_suite.dart' somewhat obsolete (i.e. in that case we need a
> different mechanism)?

Partially agree =). We definitely need to run the tests with dart2js, but we
will also need the changes in test_suite.dart of this CL. Currently polymer has
2 modes of running: a development mode (dartium only) and a deployment mode
(dartium or dart2js). The changes in this CL make it possible to test the
development mode, John is working next on making the deployment mode testable.
As you said, for the deployment mode we'll need to put compiler output
somewhere. John is making progress on that, I'll let him comment on what he is
planning to do.

> 
> I saw now that terry@ added a third_party/polymer/ShadowDOM dependency
pointing
> to an external github repository.
> Please CC ricow@+kustermann@ on such changes. It's important that we know
about
> these things. We want to minimize our external dependencies as much as we can
--
> usually we have internal mirrors of repositories.
> Now all our builders are fetching code from github. Could easily be the case
> that github blocks us for whatever reason (e.g. it thinks we're DDos-ing it
...
> [has happened before with googlecode]) -- and this would make all builders
red!
> [ricow added https://code.google.com/p/dart/wiki/ThirdPartyAndBinaryData%5D.
> 
>
https://codereview.chromium.org/23539003/diff/23001/pkg/unittest/lib/test_con...
> File pkg/unittest/lib/test_controller.js (right):
> 
>
https://codereview.chromium.org/23539003/diff/23001/pkg/unittest/lib/test_con...
> pkg/unittest/lib/test_controller.js:27: if (navigator.webkitStartDart &&
> !window.HTMLImports) {
> Please elaborate here a bit (in particular mention the interaction with
polymer)
> -- as time goes by these kind of things can easily be forgotten.
> 
>
https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/sam...
> File samples/tests/samples/samples.status (right):
> 
>
https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/sam...
> samples/tests/samples/samples.status:46: todomvc/*: Skip
> If you've Skip here, please file a bug and add the issue number. That's the
> normal procedure we use.
> 
>
https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/tod...
> File samples/tests/samples/todomvc/listorder_test.html (right):
> 
>
https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/tod...
> samples/tests/samples/todomvc/listorder_test.html:12: <link rel="stylesheet"
> href="../../../third_party/todomvc/web/base.css">
> What is this 'third_party/todomvc'
> 
> I cannot remember that I've seen additions to third_party.
> Is that from github?

[Jennifer Messerly, 2013-08-30 18:07:36]

I think Siggi addressed most of the comments, thanks!
I just want to reiterate -- we have moved everything. There is no GitHub stuff
except what the Polymer team produces on github.com/polymer, and even in those
cases we check in a concatenated/minified JS file (see pkg/shadow_dom).

I'll let Terry comment on the github thing since that's unrelated to this CL.
But as far as I know, we have *no* build/test dependencies on github. If the
DEPS entry itself is a problem, definitely follow up on that, we could easily do
something else.

https://codereview.chromium.org/23539003/diff/23001/pkg/unittest/lib/test_con...
File pkg/unittest/lib/test_controller.js (right):

https://codereview.chromium.org/23539003/diff/23001/pkg/unittest/lib/test_con...
pkg/unittest/lib/test_controller.js:27: if (navigator.webkitStartDart &&
!window.HTMLImports) {
On 2013/08/30 15:34:43, kustermann wrote:
> Please elaborate here a bit (in particular mention the interaction with
polymer)
> -- as time goes by these kind of things can easily be forgotten.

Attempted to clarify:

// Start Dartium/content_shell, unless we are waiting for HTML Imports to load.
// HTML Imports allows a document to link to other HTMLs documents via
// <link rel=import>. It also allows for those other documents to contain
// <script> tags, which must be run before scripts on the main page.
// We have package:html_import to polyfill this feature, and it will handle
// starting Dartium/content_shell in that case. HTML Imports is used by Polymer,
// but it could be used by itself too. See the specification:
// https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/imports/index.html

https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/sam...
File samples/tests/samples/samples.status (right):

https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/sam...
samples/tests/samples/samples.status:46: todomvc/*: Skip
On 2013/08/30 15:34:43, kustermann wrote:
> If you've Skip here, please file a bug and add the issue number. That's the
> normal procedure we use.

good catch! I did file a bug but forgot to add the comment here. Done.

https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/tod...
File samples/tests/samples/todomvc/listorder_test.html (right):

https://codereview.chromium.org/23539003/diff/23001/samples/tests/samples/tod...
samples/tests/samples/todomvc/listorder_test.html:12: <link rel="stylesheet"
href="../../../third_party/todomvc/web/base.css">
On 2013/08/30 15:34:43, kustermann wrote:
> What is this 'third_party/todomvc'
> 
> I cannot remember that I've seen additions to third_party.
> Is that from github?

fyi -- none of Polymer or its dependencies is on github anymore, except for
JavaScript polyfill code we get from the https://github.com/polymer project.

third_party/todomvc is authored by us Dart Authors, however, it is contains
third party (MIT) licensed CSS files. See:
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/samp...

[Jennifer Messerly, 2013-08-30 21:09:17]

Committed patchset #7 manually as r26943 (presubmit successful).