fixed TodoMVC tests to include deploying to JS

fixed TodoMVC tests to include deploying to JS

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

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

[Jennifer Messerly, 2013-08-30 23:01:10]

Here is testing TodoMVC deployment story: Polymer's deploy + dart2js.

https://codereview.chromium.org/23678009/diff/1/samples/samples.status
File samples/samples.status (right):

https://codereview.chromium.org/23678009/diff/1/samples/samples.status#newcode24
samples/samples.status:24: third_party/todomvc: Pass, Fail # issue 12865
right now TodoMVC seems broken on Firefox, but it might be related to the time I
happened to sync; there's some brokenness on the build bots.

[Siggi Cherem (dart-lang), 2013-08-30 23:22:13]

only minor nits below, but otherwise LGTM

https://codereview.chromium.org/23678009/diff/1/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/23678009/diff/1/pkg/polymer/lib/deploy.dart#n...
pkg/polymer/lib/deploy.dart:66: if (parentDir == dir) return null;
maybe define `var root = path.rootPrefix(dir)` upfront and compare against that
here?

https://codereview.chromium.org/23678009/diff/1/pkg/polymer/lib/deploy.dart#n...
pkg/polymer/lib/deploy.dart:95: 
delete empty line?

https://codereview.chromium.org/23678009/diff/1/pkg/polymer/lib/deploy.dart#n...
pkg/polymer/lib/deploy.dart:276: ..addOption('test', help: 'Deploy the test at
the given path.');
minor nit: maybe indicate that this is to limit deploy to run on that test only
(hopefully one day)

https://codereview.chromium.org/23678009/diff/1/samples/samples.status
File samples/samples.status (right):

https://codereview.chromium.org/23678009/diff/1/samples/samples.status#newcode24
samples/samples.status:24: third_party/todomvc: Pass, Fail # issue 12865
On 2013/08/30 23:01:10, John Messerly wrote:
> right now TodoMVC seems broken on Firefox, but it might be related to the time
I
> happened to sync; there's some brokenness on the build bots.

not sure what is the best status here (Pass, Fail), Fail, or Skip? (PassFail)
seems to say this is flaky, so I might prefer Fail or Skip.

https://codereview.chromium.org/23678009/diff/1/samples/third_party/todomvc/t...
File samples/third_party/todomvc/test/listorder_test.dart (right):

https://codereview.chromium.org/23678009/diff/1/samples/third_party/todomvc/t...
samples/third_party/todomvc/test/listorder_test.dart:58: 
revert =)

[Jennifer Messerly, 2013-09-04 04:36:40]

Thanks Siggi! PTAL

https://codereview.chromium.org/23678009/diff/1/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/23678009/diff/1/pkg/polymer/lib/deploy.dart#n...
pkg/polymer/lib/deploy.dart:66: if (parentDir == dir) return null;
On 2013/08/30 23:22:13, Siggi Cherem (dart-lang) wrote:
> maybe define `var root = path.rootPrefix(dir)` upfront and compare against
that
> here?

hmmm. I was woried that dirname does not necessarily converge to rootPrefix in
the case of relative paths:
http://api.dartlang.org/docs/releases/latest/path.html#dirname, 

however it will eventually reach a fixed point, hence checking for that.

however I could move the "path.absolute" inside this function and then it would
be safe to use rootPrefix.

WDYT?

https://codereview.chromium.org/23678009/diff/1/pkg/polymer/lib/deploy.dart#n...
pkg/polymer/lib/deploy.dart:95: 
On 2013/08/30 23:22:13, Siggi Cherem (dart-lang) wrote:
> delete empty line?

Done.

https://codereview.chromium.org/23678009/diff/1/pkg/polymer/lib/deploy.dart#n...
pkg/polymer/lib/deploy.dart:276: ..addOption('test', help: 'Deploy the test at
the given path.');
On 2013/08/30 23:22:13, Siggi Cherem (dart-lang) wrote:
> minor nit: maybe indicate that this is to limit deploy to run on that test
only
> (hopefully one day)

thought about that, was worried about length making --help unreadable since
"args" doesn't know the TERM width. I played around with it and discovered that
"\n" works. Hopefully the message is better now.

https://codereview.chromium.org/23678009/diff/1/samples/samples.status
File samples/samples.status (right):

https://codereview.chromium.org/23678009/diff/1/samples/samples.status#newcode24
samples/samples.status:24: third_party/todomvc: Pass, Fail # issue 12865
On 2013/08/30 23:22:13, Siggi Cherem (dart-lang) wrote:
> On 2013/08/30 23:01:10, John Messerly wrote:
> > right now TodoMVC seems broken on Firefox, but it might be related to the
time
> I
> > happened to sync; there's some brokenness on the build bots.
> 
> not sure what is the best status here (Pass, Fail), Fail, or Skip? (PassFail)
> seems to say this is flaky, so I might prefer Fail or Skip.

It looks like these are actually passing now. I was merged to a point where the
dart:html tree was red. Removed!

https://codereview.chromium.org/23678009/diff/1/samples/third_party/todomvc/t...
File samples/third_party/todomvc/test/listorder_test.dart (right):

https://codereview.chromium.org/23678009/diff/1/samples/third_party/todomvc/t...
samples/third_party/todomvc/test/listorder_test.dart:58: 
On 2013/08/30 23:22:13, Siggi Cherem (dart-lang) wrote:
> revert =)

Done.

[kustermann, 2013-09-04 12:02:23]

lgtm since you say it's urgent.

At some point we should really cleanup the code in test_suite.dart (it was ugly
before and your change isn't improving things ...) -- but we'll handle that.

https://codereview.chromium.org/23678009/diff/8001/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/23678009/diff/8001/pkg/polymer/lib/deploy.dar...
pkg/polymer/lib/deploy.dart:29: var args = _parseArgs(new Options().arguments);
I haven't looked at all of this, but I strongly hope that this tool will return
with nonzero exitCode if something went wrong?

https://codereview.chromium.org/23678009/diff/8001/samples/samples.status
File samples/samples.status (right):

https://codereview.chromium.org/23678009/diff/8001/samples/samples.status#new...
samples/samples.status:45: dartcombat/test/dartcombat_test: Skip
Please
- run it locally on as many runtimes as possible
- add the failing tests here
- file bugs for the failing tests
before you commit.
(First committing and then making status updates should be avoided *whenever
possible*)

I've patched in your CL, here's what I get:

dart2js-chrome: [dart2js-chrome release_ia32]:
  samples/third_party/todomvc/test/listorder_test: fail
  samples/third_party/todomvc/test/mainpage_test: fail
  samples/third_party/todomvc/test/markdone_shadow_test: fail
  samples/third_party/todomvc/test/markdone_test: fail

dart2js-ff: [dart2js-ff release_ia32]:
  samples/third_party/todomvc/test/listorder_test: fail
  samples/third_party/todomvc/test/mainpage_test: fail
  samples/third_party/todomvc/test/markdone_shadow_test: fail
  samples/third_party/todomvc/test/markdone_test: fail


Does it make sense to run these tests with analyzer/d8/jsshell/... ?
(Currently you only skip it on the VM!)

https://codereview.chromium.org/23678009/diff/8001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/23678009/diff/8001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:1179: String packageRoot =
packageRootArgument(optionsFromFile['packageRoot']);
I don't think we should support 'optionsFromFile['packageRoot']' for polymer.
This option is only used by *two* tests in the whole repository.

Using 'packageRootArgument(null)' should be fine. Then you can also remove the
'optionsFromFile' parameter.

[Siggi Cherem (dart-lang), 2013-09-04 15:23:32]

lgtm

https://codereview.chromium.org/23678009/diff/1/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/23678009/diff/1/pkg/polymer/lib/deploy.dart#n...
pkg/polymer/lib/deploy.dart:66: if (parentDir == dir) return null;
On 2013/09/04 04:36:40, John Messerly wrote:
> On 2013/08/30 23:22:13, Siggi Cherem (dart-lang) wrote:
> > maybe define `var root = path.rootPrefix(dir)` upfront and compare against
> that
> > here?
> 
> hmmm. I was woried that dirname does not necessarily converge to rootPrefix in
> the case of relative paths:
> http://api.dartlang.org/docs/releases/latest/path.html#dirname, 
> 
> however it will eventually reach a fixed point, hence checking for that.
> 
> however I could move the "path.absolute" inside this function and then it
would
> be safe to use rootPrefix.
> 
> WDYT?

I see - it seems that in that case it will converge to '.'

I like the idea of using path.absolute inside this function. Even though it's
documented, the fact that `dirname('.') == dirname('foo') == '.')` feels like
it's leaking some implementation detail.

[Jennifer Messerly, 2013-09-04 18:52:23]

https://codereview.chromium.org/23678009/diff/8001/samples/samples.status
File samples/samples.status (right):

https://codereview.chromium.org/23678009/diff/8001/samples/samples.status#new...
samples/samples.status:45: dartcombat/test/dartcombat_test: Skip
On 2013/09/04 12:02:23, kustermann wrote:
> Please
> - run it locally on as many runtimes as possible
> - add the failing tests here
> - file bugs for the failing tests
> before you commit.
> (First committing and then making status updates should be avoided *whenever
> possible*)
> 
> I've patched in your CL, here's what I get:
> 
> dart2js-chrome: [dart2js-chrome release_ia32]:
>   samples/third_party/todomvc/test/listorder_test: fail
>   samples/third_party/todomvc/test/mainpage_test: fail
>   samples/third_party/todomvc/test/markdone_shadow_test: fail
>   samples/third_party/todomvc/test/markdone_test: fail
> 
> dart2js-ff: [dart2js-ff release_ia32]:
>   samples/third_party/todomvc/test/listorder_test: fail
>   samples/third_party/todomvc/test/mainpage_test: fail
>   samples/third_party/todomvc/test/markdone_shadow_test: fail
>   samples/third_party/todomvc/test/markdone_test: fail

yeah, I'm getting that today too. Ugh! It looks like something broke since
yesterday :(. I'm investigating.

> 
> 
> Does it make sense to run these tests with analyzer/d8/jsshell/... ?
> (Currently you only skip it on the VM!)

Not sure. None of the other samples are skipped for "d8" or "jsshell". It works,
but I have no idea why :)

I did try it on all of these configurations, works great.

[Jennifer Messerly, 2013-09-05 05:07:05]

On 2013/09/04 18:52:23, John Messerly wrote:
> https://codereview.chromium.org/23678009/diff/8001/samples/samples.status
> File samples/samples.status (right):
> 
>
https://codereview.chromium.org/23678009/diff/8001/samples/samples.status#new...
> samples/samples.status:45: dartcombat/test/dartcombat_test: Skip
> On 2013/09/04 12:02:23, kustermann wrote:
> > Please
> > - run it locally on as many runtimes as possible
> > - add the failing tests here
> > - file bugs for the failing tests
> > before you commit.
> > (First committing and then making status updates should be avoided *whenever
> > possible*)
> > 
> > I've patched in your CL, here's what I get:
> > 
> > dart2js-chrome: [dart2js-chrome release_ia32]:
> >   samples/third_party/todomvc/test/listorder_test: fail
> >   samples/third_party/todomvc/test/mainpage_test: fail
> >   samples/third_party/todomvc/test/markdone_shadow_test: fail
> >   samples/third_party/todomvc/test/markdone_test: fail
> > 
> > dart2js-ff: [dart2js-ff release_ia32]:
> >   samples/third_party/todomvc/test/listorder_test: fail
> >   samples/third_party/todomvc/test/mainpage_test: fail
> >   samples/third_party/todomvc/test/markdone_shadow_test: fail
> >   samples/third_party/todomvc/test/markdone_test: fail
> 
> yeah, I'm getting that today too. Ugh! It looks like something broke since
> yesterday :(. I'm investigating.
> 
> > 
> > 
> > Does it make sense to run these tests with analyzer/d8/jsshell/... ?
> > (Currently you only skip it on the VM!)
> 
> Not sure. None of the other samples are skipped for "d8" or "jsshell". It
works,
> but I have no idea why :)
> 
> I did try it on all of these configurations, works great.

fyi -- Firefox was badly broken. The main culprit was Pete's innerHTML change.
There might've been a dart2js change as well, because a lot more code was going
through interceptors, which exposed several bugs in
Firefox+ShadowDOM+interceptors :|

I think I have them ironed out now in: https://codereview.chromium.org/23548014/
I'll wait to land this patch until that one lands, since getting Firefox another
other browsers working and tested was one of the main objectives :)

There's also a bug from the last Chrome+Blink roll, that breaks ShadowDOM on
older Chromes. Working on a fix for that too.

(This is the problem with not having tests running on the bots yet ... things
break and nobody notices ;) )

[Jennifer Messerly, 2013-09-05 23:00:14]

alright, it's passing again in firefox and chrome (*knock on wood*). Siggi and I
pair programmed the additional Polymer fixes needed after the Great InnerHTML
Breaking, and the ShadowDOM fix is landed too.

I'm going to keep an eye on the bots too, since I'm not sure if Safari and IE9
will pass. Hopefully this will just need some status file tweaks if there are
any issues in those browsers. I'll update issue 12865 depending on the outcome.

[Jennifer Messerly, 2013-09-05 23:00:50]

Committed patchset #4 manually as r27222 (presubmit successful).