Reland of Moves mojo_js_integration_tests into blink.

Reland of Moves mojo_js_integration_tests into blink. (patchset #1 id:1 of https://codereview.chromium.org/2926143002/ )

Reason for revert:
Fixed failing tests.

Original issue's description:
> Revert of Moves mojo_js_integration_tests into blink. (patchset #11 id:190001 of https://codereview.chromium.org/2734783008/ )
> 
> Reason for revert:
> Seems related to webkit failures of the JsToCpp tests. E.x. https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/builds/34530.
> 
> Original issue's description:
> > Moves mojo_js_integration_tests into blink.
> > 
> > BUG=699569
> > 
> > Review-Url: https://codereview.chromium.org/2734783008
> > Cr-Commit-Position: refs/heads/master@{#477549}
> > Committed: https://chromium.googlesource.com/chromium/src/+/dd57fa764813de09d159a232eeef1bb21b0ccc1e
> 
> TBR=jbroman@chromium.org,dpranke@chromium.org,yzshen@chromium.org,alokp@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=699569
> 
> Review-Url: https://codereview.chromium.org/2926143002
> Cr-Commit-Position: refs/heads/master@{#477653}
> Committed: https://chromium.googlesource.com/chromium/src/+/6536a56d25c245bd109e32ed49240bfc04f8890f

TBR=jbroman@chromium.org,dpranke@chromium.org,yzshen@chromium.org,jwd@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=699569
Review-Url: https://codereview.chromium.org/2920383004
Cr-Commit-Position: refs/heads/master@{#478820}
Committed: https://chromium.googlesource.com/chromium/src/+/0fac10b6b0a132a25eedf971c8ccffaa6e8a4961

[alokp, 2017-06-07 17:48:49]

Created Reland of Moves mojo_js_integration_tests into blink.

[Dirk Pranke, 2017-06-07 18:05:11]

lgtm

[alokp, 2017-06-07 18:07:30]

Dirk: For webkit_unit_tests, do I need to change any script for webkit bots to
have access to all data dependencies?

I noticed that "webkit_unit_tests_data" target just lists data dependencies,
which is exactly what I did in core/mojo:unit_tests target.

[alokp, 2017-06-07 18:27:02]

It is also not clear to me which trybots I should use to make sure this patch
does not break again on webkit bots. I tried one from master:tryserver.blink,
but it passed.

I guess I am looking for a bot that runs the test in an environment without the
chromium source tree.

[Dirk Pranke, 2017-06-07 19:38:18]

On 2017/06/07 18:27:02, alokp wrote:
> It is also not clear to me which trybots I should use to make sure this patch
> does not break again on webkit bots. I tried one from master:tryserver.blink,
> but it passed.
> 
> I guess I am looking for a bot that runs the test in an environment without
the
> chromium source tree.

webkit_unit_tests passed on your mac10.12_blink_rel change, so you should be
fine. {linux,mac,win}_chromium_rel_ng should also run the test step.

I don't fully understand the original failure or how the patch got through the
CQ. Can you point me at the fix for the tests?

[alokp, 2017-06-07 20:22:24]

On 2017/06/07 19:38:18, Dirk Pranke wrote:
> On 2017/06/07 18:27:02, alokp wrote:
> > It is also not clear to me which trybots I should use to make sure this
patch
> > does not break again on webkit bots. I tried one from
master:tryserver.blink,
> > but it passed.
> > 
> > I guess I am looking for a bot that runs the test in an environment without
> the
> > chromium source tree.
> 
> webkit_unit_tests passed on your mac10.12_blink_rel change, so you should be
> fine. {linux,mac,win}_chromium_rel_ng should also run the test step.
> 
> I don't fully understand the original failure or how the patch got through the
> CQ. Can you point me at the fix for the tests?

I have not yet fixed anything. This is still the original patch that I was
planning to send to various trybots to understand the failure. The original
failed run is here:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build...

[Dirk Pranke, 2017-06-07 20:24:55]

On 2017/06/07 20:22:24, alokp wrote:
> On 2017/06/07 19:38:18, Dirk Pranke wrote:
> > On 2017/06/07 18:27:02, alokp wrote:
> > > It is also not clear to me which trybots I should use to make sure this
> patch
> > > does not break again on webkit bots. I tried one from
> master:tryserver.blink,
> > > but it passed.
> > > 
> > > I guess I am looking for a bot that runs the test in an environment
without
> > the
> > > chromium source tree.
> > 
> > webkit_unit_tests passed on your mac10.12_blink_rel change, so you should be
> > fine. {linux,mac,win}_chromium_rel_ng should also run the test step.
> > 
> > I don't fully understand the original failure or how the patch got through
the
> > CQ. Can you point me at the fix for the tests?
> 
> I have not yet fixed anything. This is still the original patch that I was
> planning to send to various trybots to understand the failure. The original
> failed run is here:
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build...

Did it only fail on 10.10? If so, you could try the mac10.10_blink_rel bot as
well ...

[alokp, 2017-06-07 20:35:09]

On 2017/06/07 20:24:55, Dirk Pranke wrote:
> On 2017/06/07 20:22:24, alokp wrote:
> > On 2017/06/07 19:38:18, Dirk Pranke wrote:
> > > On 2017/06/07 18:27:02, alokp wrote:
> > > > It is also not clear to me which trybots I should use to make sure this
> > patch
> > > > does not break again on webkit bots. I tried one from
> > master:tryserver.blink,
> > > > but it passed.
> > > > 
> > > > I guess I am looking for a bot that runs the test in an environment
> without
> > > the
> > > > chromium source tree.
> > > 
> > > webkit_unit_tests passed on your mac10.12_blink_rel change, so you should
be
> > > fine. {linux,mac,win}_chromium_rel_ng should also run the test step.
> > > 
> > > I don't fully understand the original failure or how the patch got through
> the
> > > CQ. Can you point me at the fix for the tests?
> > 
> > I have not yet fixed anything. This is still the original patch that I was
> > planning to send to various trybots to understand the failure. The original
> > failed run is here:
> >
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build...
> 
> Did it only fail on 10.10? If so, you could try the mac10.10_blink_rel bot as
> well ...

Failed on 10.12 as well:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.12/build...

But again I am not sure if chromium.webkit/builders/WebKit%20Mac10.12 (the bot
where it failed) is the same as tryserver.blink/builders/mac10.12_blink_rel (the
bot that runs the tryjob).

[Dirk Pranke, 2017-06-07 23:33:05]

On 2017/06/07 20:35:09, alokp wrote:
> On 2017/06/07 20:24:55, Dirk Pranke wrote:
> > On 2017/06/07 20:22:24, alokp wrote:
> > > On 2017/06/07 19:38:18, Dirk Pranke wrote:
> > > > On 2017/06/07 18:27:02, alokp wrote:
> > > > > It is also not clear to me which trybots I should use to make sure
this
> > > patch
> > > > > does not break again on webkit bots. I tried one from
> > > master:tryserver.blink,
> > > > > but it passed.
> > > > > 
> > > > > I guess I am looking for a bot that runs the test in an environment
> > without
> > > > the
> > > > > chromium source tree.
> > > > 
> > > > webkit_unit_tests passed on your mac10.12_blink_rel change, so you
should
> be
> > > > fine. {linux,mac,win}_chromium_rel_ng should also run the test step.
> > > > 
> > > > I don't fully understand the original failure or how the patch got
through
> > the
> > > > CQ. Can you point me at the fix for the tests?
> > > 
> > > I have not yet fixed anything. This is still the original patch that I was
> > > planning to send to various trybots to understand the failure. The
original
> > > failed run is here:
> > >
> >
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build...
> > 
> > Did it only fail on 10.10? If so, you could try the mac10.10_blink_rel bot
as
> > well ...
> 
> Failed on 10.12 as well:
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.12/build...
> 
> But again I am not sure if chromium.webkit/builders/WebKit%20Mac10.12 (the bot
> where it failed) is the same as tryserver.blink/builders/mac10.12_blink_rel
(the
> bot that runs the tryjob).

Yes, it's supposed to be the same. For some reason, it looks like we're building
with full symbols on the waterfall bot
and minimal symbols on the tryserver, but it seems unlikely that that would be
the problem.

[alokp, 2017-06-07 23:51:11]

> 
> Yes, it's supposed to be the same. For some reason, it looks like we're
building
> with full symbols on the waterfall bot
> and minimal symbols on the tryserver, but it seems unlikely that that would be
> the problem.

I see a bunch of differences in build steps on the two bots:

1. Trybot has a compile step, where as waterfall bot has extract build step. It
looks like the waterfall bot does not have chromium source tree.

2. Trybot has a isolate_tests step, which curiously has this in stdout:
11:17:18.006437
PushDirectory(/b/c/b/mac_layout/src/third_party/WebKit/LayoutTests/images/resources/)
= 201 files
11:17:18.007856
PushDirectory(/b/c/b/mac_layout/src/third_party/WebKit/Source/platform/image-decoders/testing/)
= 8 files
11:17:18.009357
PushDirectory(/b/c/b/mac_layout/src/third_party/WebKit/Source/platform/testing/data/)
= 3 files
11:17:18.299301 PushDirectory(/b/c/b/mac_layout/src/net/data/) = 1666 files
11:17:18.299340 PushDirectory(/b/c/b/mac_layout/src/net/data/) = 1666 files
11:17:18.300621
PushDirectory(/b/c/b/mac_layout/src/third_party/WebKit/Source/core/paint/test_data/)
= 5 files
11:17:18.361231
PushDirectory(/b/c/b/mac_layout/src/third_party/WebKit/Source/web/tests/data/) =
583 files

It seems like there is a step to explicitly copy test data on trybots, which is
missing on the waterfall.

[Dirk Pranke, 2017-06-08 00:49:33]

On 2017/06/07 23:51:11, alokp wrote:
> > 
> > Yes, it's supposed to be the same. For some reason, it looks like we're
> building
> > with full symbols on the waterfall bot
> > and minimal symbols on the tryserver, but it seems unlikely that that would
be
> > the problem.
> 
> I see a bunch of differences in build steps on the two bots:
> 
> 1. Trybot has a compile step, where as waterfall bot has extract build step.
It
> looks like the waterfall bot does not have chromium source tree.
> 
> 2. Trybot has a isolate_tests step, which curiously has this in stdout:
> 11:17:18.006437
>
PushDirectory(/b/c/b/mac_layout/src/third_party/WebKit/LayoutTests/images/resources/)
> = 201 files
> 11:17:18.007856
>
PushDirectory(/b/c/b/mac_layout/src/third_party/WebKit/Source/platform/image-decoders/testing/)
> = 8 files
> 11:17:18.009357
>
PushDirectory(/b/c/b/mac_layout/src/third_party/WebKit/Source/platform/testing/data/)
> = 3 files
> 11:17:18.299301 PushDirectory(/b/c/b/mac_layout/src/net/data/) = 1666 files
> 11:17:18.299340 PushDirectory(/b/c/b/mac_layout/src/net/data/) = 1666 files
> 11:17:18.300621
>
PushDirectory(/b/c/b/mac_layout/src/third_party/WebKit/Source/core/paint/test_data/)
> = 5 files
> 11:17:18.361231
> PushDirectory(/b/c/b/mac_layout/src/third_party/WebKit/Source/web/tests/data/)
=
> 583 files
> 
> It seems like there is a step to explicitly copy test data on trybots, which
is
> missing on the waterfall.

Ah, okay, I see what happened, I think.

On mac_chromium_rel_ng, the test is run under swarming (in an isolate). On the
waterfall bot,
you're correct that the test is run on a separate test-only bot that only has a
subset of the checkout.
On the tryserver, it has the full checkout.

In theory, if the test passes under swarming, it should run anywhere. However,
it's possible that
you're pulling in something as a data dependency that isn't also being included
in the zip file
that the tester gets, and so the test might fail there.

You should check the list of files here:

https://cs.chromium.org/chromium/build/scripts/slave/zip_build.py?l=123

[alokp, 2017-06-08 05:07:43]

Thanks Dirk!

Yup this is definitely one problem. The generated JS bindings for
third_party/WebKit/Source/core/mojo/tests/JsToCpp.mojom is not being included.
Maintaining data dependency in two places (in separate repositories) is a pain.
Should I just include gen/third_party/WebKit/Source?

How about source tree? Is it available on the testers? I have a data dependency
on a JS file in source tree:
	third_party/WebKit/Source/core/mojo/tests/JsToCppTest.js

Is there a trybot that runs tests in the same environment as waterfall testers?
CQ should ideally catch these issues. It seems like the ad hoc zip_build.py
solution is no longer suitable for mojo world with lots of auto-generated data
dependencies.

[Dirk Pranke, 2017-06-08 21:23:19]

On 2017/06/08 05:07:43, alokp wrote:
> Thanks Dirk!
> 
> Yup this is definitely one problem. The generated JS bindings for
> third_party/WebKit/Source/core/mojo/tests/JsToCpp.mojom is not being included.
> Maintaining data dependency in two places (in separate repositories) is a
pain.
> Should I just include gen/third_party/WebKit/Source?

Yes, that's fine.

> 
> How about source tree? Is it available on the testers? I have a data
dependency
> on a JS file in source tree:
> 	third_party/WebKit/Source/core/mojo/tests/JsToCppTest.js

That's okay, as long as it is mentioned in the data_deps, but it might be better
to copy
the file into out/.

The testers do not have a full checkout, so you'd have to update the zip_build
script
with whatever directories or files you need.
> 
> Is there a trybot that runs tests in the same environment as waterfall
testers?

No.

> CQ should ideally catch these issues. 

Yes, ideally :).

> It seems like the ad hoc zip_build.py
> solution is no longer suitable for mojo world with lots of auto-generated data
> dependencies.

We would like to get rid of the split builder/tester configurations and just run
all of
the tests under swarming instead, but we're not quite there yet.

As an intermediary measure, we could also change the archive_build/extract_build
process
to be more integrated with the data_deps and isolates, but in some sense that's
throwaway
work and it would be lower priority than the work we're currently doing to just
move to
swarming.

[alokp, 2017-06-08 21:44:57]

> > Should I just include gen/third_party/WebKit/Source?
> 
> Yes, that's fine.

OK - I will send a patch for that shortly.

> We would like to get rid of the split builder/tester configurations and just
run
> all of
> the tests under swarming instead, but we're not quite there yet.
> 
> As an intermediary measure, we could also change the
archive_build/extract_build
> process
> to be more integrated with the data_deps and isolates, but in some sense
that's
> throwaway
> work and it would be lower priority than the work we're currently doing to
just
> move to
> swarming.

That makes sense. Good to know that we are deprecating this split config.

[alokp, 2017-06-12 22:02:56]

Dirk: Is it OK to land this patch now?

Infra patch landed this morning:
https://chromium-review.googlesource.com/c/529765/
How long does it take for infra changes to take effect?

[Dirk Pranke, 2017-06-12 22:05:18]

On 2017/06/12 22:02:56, alokp wrote:
> Dirk: Is it OK to land this patch now?

Yes, I think we can give it another shot now.

> Infra patch landed this morning:
> https://chromium-review.googlesource.com/c/529765/
> How long does it take for infra changes to take effect?

Most things, including your change, get picked up on the next build.

[alokp, 2017-06-12 23:33:24]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2017-06-12 23:33:49]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 23:34:18]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1497310404077730, "parent_rev":
"f1e4c0cc3c76b155337800383e54537a88fcdfe7", "commit_rev":
"0fac10b6b0a132a25eedf971c8ccffaa6e8a4961"}

[commit-bot: I haz the power, 2017-06-12 23:34:31]

Description was changed from

==========
Reland of Moves mojo_js_integration_tests into blink. (patchset #1 id:1 of
https://codereview.chromium.org/2926143002/ )

Reason for revert:
Fixed failing tests.

Original issue's description:
> Revert of Moves mojo_js_integration_tests into blink. (patchset #11 id:190001
of https://codereview.chromium.org/2734783008/ )
> 
> Reason for revert:
> Seems related to webkit failures of the JsToCpp tests. E.x.
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build....
> 
> Original issue's description:
> > Moves mojo_js_integration_tests into blink.
> > 
> > BUG=699569
> > 
> > Review-Url: https://codereview.chromium.org/2734783008
> > Cr-Commit-Position: refs/heads/master@{#477549}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/dd57fa764813de09d159a232eeef...
> 
>
TBR=jbroman@chromium.org,dpranke@chromium.org,yzshen@chromium.org,alokp@chrom...
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=699569
> 
> Review-Url: https://codereview.chromium.org/2926143002
> Cr-Commit-Position: refs/heads/master@{#477653}
> Committed:
https://chromium.googlesource.com/chromium/src/+/6536a56d25c245bd109e32ed4924...

TBR=jbroman@chromium.org,dpranke@chromium.org,yzshen@chromium.org,jwd@chromiu...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=699569
==========

to

==========
Reland of Moves mojo_js_integration_tests into blink. (patchset #1 id:1 of
https://codereview.chromium.org/2926143002/ )

Reason for revert:
Fixed failing tests.

Original issue's description:
> Revert of Moves mojo_js_integration_tests into blink. (patchset #11 id:190001
of https://codereview.chromium.org/2734783008/ )
> 
> Reason for revert:
> Seems related to webkit failures of the JsToCpp tests. E.x.
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build....
> 
> Original issue's description:
> > Moves mojo_js_integration_tests into blink.
> > 
> > BUG=699569
> > 
> > Review-Url: https://codereview.chromium.org/2734783008
> > Cr-Commit-Position: refs/heads/master@{#477549}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/dd57fa764813de09d159a232eeef...
> 
>
TBR=jbroman@chromium.org,dpranke@chromium.org,yzshen@chromium.org,alokp@chrom...
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=699569
> 
> Review-Url: https://codereview.chromium.org/2926143002
> Cr-Commit-Position: refs/heads/master@{#477653}
> Committed:
https://chromium.googlesource.com/chromium/src/+/6536a56d25c245bd109e32ed4924...

TBR=jbroman@chromium.org,dpranke@chromium.org,yzshen@chromium.org,jwd@chromiu...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=699569

Review-Url: https://codereview.chromium.org/2920383004
Cr-Commit-Position: refs/heads/master@{#478820}
Committed:
https://chromium.googlesource.com/chromium/src/+/0fac10b6b0a132a25eedf971c8cc...
==========

[commit-bot: I haz the power, 2017-06-12 23:34:32]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/0fac10b6b0a132a25eedf971c8cc...

[tzik, 2017-06-13 03:15:32]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2939663002/ by tzik@chromium.org.

The reason for reverting is: This CL seems to break webkit_unittests.
The error log is:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build...

The tests below were timed out:
JsToCppTest.BackPointer
JsToCppTest.BitFlip
JsToCppTest.Echo
JsToCppTest.Ping.