Stage harmony trailing commas

Stage harmony trailing commas

BUG=v8:5051
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/87ca9283b05e5a37539c5ee52efc6b9c8848aa12
Cr-Commit-Position: refs/heads/master@{#40942}

[jwolfe, 2016-11-09 20:36:37]

Description was changed from

==========
Stage harmony trailing commas

BUG=v8:5051
==========

to

==========
Stage harmony trailing commas

BUG=v8:5051
==========

[jwolfe, 2016-11-09 20:36:37]

jwolfe@igalia.com changed reviewers:
+ littledan@chromium.org

[Dan Ehrenberg, 2016-11-09 21:20:54]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 21:21:03]

Dry run: 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, 2016-11-09 21:40:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 21:40:42]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[Dan Ehrenberg, 2016-11-09 21:51:18]

Could you change the test262 status file as part of this CL?

[jwolfe, 2016-11-10 21:46:42]

On 2016/11/09 21:51:18, Dan Ehrenberg wrote:
> Could you change the test262 status file as part of this CL?

Coming soon. I've submitted a PR to test262 to fix some bugs also.
https://github.com/tc39/test262/pull/787

Should I include the DEPS update in this CL when the test262 issues are
resolved? Or should that be its own CL?

[Dan Ehrenberg, 2016-11-10 21:55:55]

On 2016/11/10 at 21:46:42, jwolfe wrote:
> On 2016/11/09 21:51:18, Dan Ehrenberg wrote:
> > Could you change the test262 status file as part of this CL?
> 
> Coming soon. I've submitted a PR to test262 to fix some bugs also.
https://github.com/tc39/test262/pull/787
> 
> Should I include the DEPS update in this CL when the test262 issues are
resolved? Or should that be its own CL?

The test262 roll will pull in other changes. I can do a roll to enable you to do
this side of the change if it's blocking.

[caitp, 2016-11-10 22:04:52]

On 2016/11/10 21:55:55, Dan Ehrenberg wrote:
> On 2016/11/10 at 21:46:42, jwolfe wrote:
> > On 2016/11/09 21:51:18, Dan Ehrenberg wrote:
> > > Could you change the test262 status file as part of this CL?
> > 
> > Coming soon. I've submitted a PR to test262 to fix some bugs also.
> https://github.com/tc39/test262/pull/787
> > 
> > Should I include the DEPS update in this CL when the test262 issues are
> resolved? Or should that be its own CL?
> 
> The test262 roll will pull in other changes. I can do a roll to enable you to
do
> this side of the change if it's blocking.

Alternatively, just mark the tests that fail as failing, and those can be fixed
during the roll.

[caitp, 2016-11-10 22:05:40]

On 2016/11/10 22:04:52, caitp wrote:
> On 2016/11/10 21:55:55, Dan Ehrenberg wrote:
> > On 2016/11/10 at 21:46:42, jwolfe wrote:
> > > On 2016/11/09 21:51:18, Dan Ehrenberg wrote:
> > > > Could you change the test262 status file as part of this CL?
> > > 
> > > Coming soon. I've submitted a PR to test262 to fix some bugs also.
> > https://github.com/tc39/test262/pull/787
> > > 
> > > Should I include the DEPS update in this CL when the test262 issues are
> > resolved? Or should that be its own CL?
> > 
> > The test262 roll will pull in other changes. I can do a roll to enable you
to
> do
> > this side of the change if it's blocking.
> 
> Alternatively, just mark the tests that fail as failing, and those can be
fixed
> during the roll.

... or vice versa, I guess?

[jwolfe, 2016-11-11 17:37:30]

If i mark tests as failing, I'd need an issue number. What issue would i
reference if the tests are failing due to their own problems rather than v8's
problems?

[caitp, 2016-11-11 18:01:27]

On 2016/11/11 17:37:30, jwolfe wrote:
> If i mark tests as failing, I'd need an issue number. What issue would i
> reference if the tests are failing due to their own problems rather than v8's
> problems?

In this case, I think you can track the failures in. 8:5051 just fine, but if
you think they aren't directly related to this, use a new or different bug at
your discretion to track it, imo.

[jwolfe, 2016-11-11 18:10:50]

The CQ bit was checked by jwolfe@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 18:11:00]

Dry run: 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, 2016-11-11 18:11:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 18:11:05]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[jwolfe, 2016-11-11 18:47:55]

Description was changed from

==========
Stage harmony trailing commas

BUG=v8:5051
==========

to

==========
Stage harmony trailing commas

BUG=v8:5051
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jwolfe, 2016-11-11 18:49:19]

Here's the status file changed to show 3 failing tests.

Meanwhile, the PR linked above has been merged into test262 master. I think it
makes sense to update the DEPS as a separate CL.

I've got a suspicion that some blink tests are going to need updating with this
change, so I've added a directive to the issue to run those tests. Meanwhile,
I'm attempting (not very successfully) to run the tests locally.

[caitp, 2016-11-11 19:12:19]

On 2016/11/11 18:49:19, jwolfe wrote:
> Here's the status file changed to show 3 failing tests.
> 
> Meanwhile, the PR linked above has been merged into test262 master. I think it
> makes sense to update the DEPS as a separate CL.
> 
> I've got a suspicion that some blink tests are going to need updating with
this
> change, so I've added a directive to the issue to run those tests. Meanwhile,
> I'm attempting (not very successfully) to run the tests locally.

There used to be a way to run layout tests with the v8 patch applied, which
would check that for us. I think it's still possible, but can't recall how. Dan,
any pointers on that?

[caitp, 2016-11-11 19:13:17]

On 2016/11/11 19:12:19, caitp wrote:
> On 2016/11/11 18:49:19, jwolfe wrote:
> > Here's the status file changed to show 3 failing tests.
> > 
> > Meanwhile, the PR linked above has been merged into test262 master. I think
it
> > makes sense to update the DEPS as a separate CL.
> > 
> > I've got a suspicion that some blink tests are going to need updating with
> this
> > change, so I've added a directive to the issue to run those tests.
Meanwhile,
> > I'm attempting (not very successfully) to run the tests locally.
> 
> There used to be a way to run layout tests with the v8 patch applied, which
> would check that for us. I think it's still possible, but can't recall how.
Dan,
> any pointers on that?

"run layout tests with the v8 patch applied" on trybots, I mean..

[jwolfe, 2016-11-11 20:22:44]

PATL.

Well, I'm not sure I know what "layout tests" are, but I ran this command in
chromium/src:

python third_party/WebKit/Tools/Scripts/run-webkit-tests -t Release

I ran it with and without my patch, and the differences between those two seem
unrelated to my change. I'm not a lot more confident that this patch will be
accepted by all the bots.

[jwolfe, 2016-11-11 20:24:29]

On 2016/11/11 20:22:44, jwolfe wrote:
> I'm not a lot more confident
Sorry, I'm *now* a lot more confident

[Dan Ehrenberg, 2016-11-11 21:02:36]

On 2016/11/11 at 20:24:29, jwolfe wrote:
> On 2016/11/11 20:22:44, jwolfe wrote:
> > I'm not a lot more confident
> Sorry, I'm *now* a lot more confident

Are the differences deterministic, or random? If there are flakes, they can be
marked. I'd they are new failures that are justified by your patch for
correctness reasons, you should mark the tests as NeedsManualRebaseline in the
status file. If they are unrelated-looking, deterministic, unexpected failures,
they deserve some more investigation.

[jwolfe, 2016-11-11 21:19:56]

(I wonder how badly formatted this message will be. let's try it.)

Without my patch:

Retrying 43 unexpected failures, attempt 3 of 3...
[1/43] webaudio/osc-sawtooth-sweep-snr.html failed unexpectedly (text diff)
[2/43] fast/text/international/combining-marks-position.html failed unexpectedly
(text diff)
[3/43] fast/dom/Window/window-resize.html failed unexpectedly (text diff)
[4/43] webaudio/periodicwave-contexts.html failed unexpectedly (text diff)
[5/43] http/tests/inspector-protocol/network-data-length.html failed
unexpectedly (text diff)
[6/43] inspector-protocol/layout-fonts/languages-emoji-rare-glyphs.html failed
unexpectedly (text diff)
[7/43] webaudio/osc-square-sweep-snr.html failed unexpectedly (text diff)
[8/43] fast/forms/calendar-picker/calendar-picker-appearance-ar.html failed
unexpectedly (image diff)
[9/43] fast/forms/select-popup/popup-menu-position.html failed unexpectedly
(text diff)
[10/43] paint/invalidation/continuation-after-outline.html failed unexpectedly
(text diff, image diff)
[11/43] webaudio/osc-custom-sweep-snr.html failed unexpectedly (text diff)
[12/43] http/tests/misc/bad-charset-alias.html failed unexpectedly (text diff)
[13/43] svg/W3C-SVG-1.1/text-intro-01-t.svg failed unexpectedly (text diff,
image diff)
[14/43]
fast/forms/suggestion-picker/time-suggestion-picker-appearance-locale-hebrew.html
failed unexpectedly (image diff)
[15/43] svg/text/non-bmp-positioning-lists.svg failed unexpectedly (text diff,
image diff)
[16/43] webaudio/osc-triangle-sweep-snr.html failed unexpectedly (text diff)
[17/43] fast/css/line-height-determined-by-primary-font.html failed unexpectedly
(image diff)
[18/43] media/media-play-promise.html failed unexpectedly (renderer crashed)
[19/43] fast/text/international/khmer-selection.html failed unexpectedly (text
diff, image diff)
[20/43] fast/text/decorations-with-text-combine.html failed unexpectedly (text
diff, image diff)
[21/43] fast/dom/52776.html failed unexpectedly (text diff, image diff)
[22/43] svg/W3C-SVG-1.1/text-intro-03-b.svg failed unexpectedly (text diff,
image diff)
[23/43] fast/forms/calendar-picker/calendar-picker-appearance-required-ar.html
failed unexpectedly (image diff)
[24/43] webaudio/osc-sine-sweep-snr.html failed unexpectedly (text diff)
[25/43] svg/W3C-SVG-1.1/text-align-08-b.svg failed unexpectedly (text diff,
image diff)
[26/43] fast/text/emphasis-combined-text.html failed unexpectedly (text diff,
image diff)
[27/43]
virtual/scalefactor200withzoom/fast/hidpi/static/data-suggestion-picker-appearance.html
failed unexpectedly (image diff)
[28/43] fast/text/international/thai-line-breaks.html failed unexpectedly (image
diff)
[29/43] svg/W3C-SVG-1.1/text-intro-04-t.svg failed unexpectedly (text diff,
image diff)
[30/43]
virtual/scalefactor200/fast/hidpi/static/data-suggestion-picker-appearance.html
failed unexpectedly (image diff)
[31/43] fast/text/justify-ideograph-simple.html failed unexpectedly (text diff,
image diff)
[32/43] fast/text/international/plane2.html failed unexpectedly (text diff,
image diff)
[33/43] fast/text/font-fallback.html failed unexpectedly (text diff, image diff)
[35/43] fast/text/justify-ideograph-complex.html failed unexpectedly (text diff,
image diff)
[36/43] fast/text/color-emoji.html failed unexpectedly (text diff, image diff)
[37/43] fast/text/midword-break-before-surrogate-pair.html failed unexpectedly
(text diff, image diff)
[38/43] fast/text/emphasis-vertical.html failed unexpectedly (text diff, image
diff)
[39/43] http/tests/notifications/serviceworker-notification-properties.html
failed unexpectedly (asserts failed)
[40/43] fast/text/justify-ideograph-vertical.html failed unexpectedly (text
diff, image diff)
[41/43] fast/text/atsui-multiple-renderers.html failed unexpectedly (image diff)
[42/43] fast/text/emphasis.html failed unexpectedly (text diff, image diff)
[43/43] fast/text/vertical-surrogate-pair.html failed unexpectedly (text diff,
image diff)


With my patch:

Retrying 41 unexpected failures, attempt 3 of 3...
[1/41] fast/dom/Window/window-resize.html failed unexpectedly (text diff)
[2/41] webaudio/osc-sawtooth-sweep-snr.html failed unexpectedly (text diff)
[3/41] fast/text/international/combining-marks-position.html failed unexpectedly
(text diff)
[4/41] webaudio/periodicwave-contexts.html failed unexpectedly (text diff)
[5/41] inspector-protocol/layout-fonts/languages-emoji-rare-glyphs.html failed
unexpectedly (text diff)
[6/41] fast/forms/select-popup/popup-menu-position.html failed unexpectedly
(text diff)
[7/41] http/tests/inspector-protocol/network-data-length.html failed
unexpectedly (text diff)
[8/41] webaudio/osc-square-sweep-snr.html failed unexpectedly (text diff)
[9/41] fast/forms/calendar-picker/calendar-picker-appearance-ar.html failed
unexpectedly (image diff)
[10/41] http/tests/misc/bad-charset-alias.html failed unexpectedly (text diff)
[11/41] svg/W3C-SVG-1.1/text-intro-01-t.svg failed unexpectedly (text diff,
image diff)
[12/41] fast/dom/52776.html failed unexpectedly (text diff, image diff)
[13/41]
fast/forms/suggestion-picker/time-suggestion-picker-appearance-locale-hebrew.html
failed unexpectedly (image diff)
[14/41] webaudio/osc-custom-sweep-snr.html failed unexpectedly (text diff)
[15/41] fast/css/line-height-determined-by-primary-font.html failed unexpectedly
(image diff)
[16/41] paint/invalidation/continuation-after-outline.html failed unexpectedly
(text diff, image diff)
[17/41] fast/text/international/khmer-selection.html failed unexpectedly (text
diff, image diff)
[18/41] svg/text/non-bmp-positioning-lists.svg failed unexpectedly (text diff,
image diff)
[19/41] fast/text/decorations-with-text-combine.html failed unexpectedly (text
diff, image diff)
[20/41] webaudio/osc-triangle-sweep-snr.html failed unexpectedly (text diff)
[21/41] svg/W3C-SVG-1.1/text-intro-03-b.svg failed unexpectedly (text diff,
image diff)
[22/41] fast/forms/calendar-picker/calendar-picker-appearance-required-ar.html
failed unexpectedly (image diff)
[23/41] webaudio/osc-sine-sweep-snr.html failed unexpectedly (text diff)
[24/41]
virtual/scalefactor200/fast/hidpi/static/data-suggestion-picker-appearance.html
failed unexpectedly (image diff)
[25/41] svg/W3C-SVG-1.1/text-align-08-b.svg failed unexpectedly (text diff,
image diff)
[26/41] fast/text/emphasis-combined-text.html failed unexpectedly (text diff,
image diff)
[27/41] fast/text/international/thai-line-breaks.html failed unexpectedly (image
diff)
[28/41]
virtual/scalefactor200withzoom/fast/hidpi/static/data-suggestion-picker-appearance.html
failed unexpectedly (image diff)
[29/41] svg/W3C-SVG-1.1/text-intro-04-t.svg failed unexpectedly (text diff,
image diff)
[30/41] fast/text/international/plane2.html failed unexpectedly (text diff,
image diff)
[31/41] fast/text/justify-ideograph-simple.html failed unexpectedly (text diff,
image diff)
[32/41] fast/text/font-fallback.html failed unexpectedly (text diff, image diff)
[33/41] fast/text/justify-ideograph-complex.html failed unexpectedly (text diff,
image diff)
[34/41] fast/text/color-emoji.html failed unexpectedly (text diff, image diff)
[35/41] fast/text/midword-break-before-surrogate-pair.html failed unexpectedly
(text diff, image diff)
[36/41] fast/text/emphasis-vertical.html failed unexpectedly (text diff, image
diff)
[37/41] fast/text/justify-ideograph-vertical.html failed unexpectedly (text
diff, image diff)
[38/41] fast/text/atsui-multiple-renderers.html failed unexpectedly (image diff)
[39/41] fast/text/emphasis.html failed unexpectedly (text diff, image diff)
[40/41] fast/text/vertical-surrogate-pair.html failed unexpectedly (text diff,
image diff)
[41/41] fast/mediacapturefromelement/HTMLMediaElementCapture-capture.html failed
unexpectedly (test timed out)


My patch seems to have fixed these two:

http/tests/notifications/serviceworker-notification-properties.html failed
unexpectedly (asserts failed)
media/media-play-promise.html failed unexpectedly (renderer crashed)

My patch seems to have broken this:

fast/mediacapturefromelement/HTMLMediaElementCapture-capture.html failed
unexpectedly (test timed out)


None of those three differences make sense to me, so they all seem random. Also
~40 failures regardless of my patch seems like my environment is misconfigured
or not supported or something. Either that or there's just a lot of broken/flaky
tests in this test suite.

[jwolfe, 2016-11-11 21:21:15]

On 2016/11/11 21:19:56, jwolfe wrote:
> (I wonder how badly formatted this message will be. let's try it.)
Here's the raw text: http://paste.ubuntu.com/23462915/

[caitp, 2016-11-11 22:10:20]

On 2016/11/11 21:21:15, jwolfe wrote:
> On 2016/11/11 21:19:56, jwolfe wrote:
> > (I wonder how badly formatted this message will be. let's try it.)
> Here's the raw text: http://paste.ubuntu.com/23462915/

Some Blink tests will almost always fail when run locally, for lots of reasons
(incorrect fonts, slow i/o or cpu, wrong system time, etc). Better to run them
on trybots where this stuff doesn't show up.

[Dan Ehrenberg, 2016-11-11 22:11:19]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 22:11:28]

Dry run: 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, 2016-11-11 22:11:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 22:11:42]

Dry run: CQ has no permission to schedule in bucket tryserver.blink

[Dan Ehrenberg, 2016-11-11 22:14:52]

Description was changed from

==========
Stage harmony trailing commas

BUG=v8:5051
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Stage harmony trailing commas

BUG=v8:5051
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Dan Ehrenberg, 2016-11-11 22:14:57]

The CQ bit was checked by littledan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 22:14:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jwolfe, 2016-11-11 23:08:46]

On 2016/11/11 22:10:20, caitp wrote:
> Better to run them on trybots where this stuff doesn't show up.
I wish I could! I tried, but was denied because I'm not a v8 committer.

Thanks Dan for running the try bot dry run.

[commit-bot: I haz the power, 2016-11-11 23:15:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 23:15:41]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2016-11-11 23:24:37]

lgtm

The trybots all passed

[jwolfe, 2016-11-11 23:40:18]

The CQ bit was checked by jwolfe@igalia.com

[commit-bot: I haz the power, 2016-11-11 23:40:22]

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, 2016-11-11 23:42:46]

Committed patchset #2 (id:20001)

[adamk, 2016-11-11 23:46:52]

As an aside: staging something should never break blink tests (or Chromium tests
for that matter), since none of those tests run with the "--harmony" flag
enabled (though it never hurts to check).

[commit-bot: I haz the power, 2016-11-17 22:31:28]

Description was changed from

==========
Stage harmony trailing commas

BUG=v8:5051
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Stage harmony trailing commas

BUG=v8:5051
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/87ca9283b05e5a37539c5ee52efc6b9c8848aa12
Cr-Commit-Position: refs/heads/master@{#40942}
==========

[commit-bot: I haz the power, 2016-11-17 22:31:29]

Patchset 2 (id:??) landed as
https://crrev.com/87ca9283b05e5a37539c5ee52efc6b9c8848aa12
Cr-Commit-Position: refs/heads/master@{#40942}