Do not change the case of titles in Utils.setResultInTitle().

Do not change the case of titles in Utils.setResultInTitle().

The previous behavior causes mentions of the 'ended' event to
immediately end MSE tests (successfully). This is true even if
the actual call was to Utils.failTest().

BUG=592067
Committed: https://crrev.com/6eb52c23c889bba614b089ca0952f419af27412e
Cr-Commit-Position: refs/heads/master@{#379382}

[sandersd (OOO until July 31), 2016-03-02 22:34:06]

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

[sandersd (OOO until July 31), 2016-03-02 22:34:24]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-03-02 22:34:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1755403002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1755403002/20001

[sandersd (OOO until July 31), 2016-03-02 23:30:34]

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

[commit-bot: I haz the power, 2016-03-02 23:31:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1755403002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1755403002/40001

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

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

[commit-bot: I haz the power, 2016-03-03 00:24:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sandersd (OOO until July 31), 2016-03-03 00:59:11]

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

[commit-bot: I haz the power, 2016-03-03 00:59:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1755403002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1755403002/60001

[commit-bot: I haz the power, 2016-03-03 02:27:30]

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

[commit-bot: I haz the power, 2016-03-03 02:27:31]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2016-03-03 02:30:12]

sandersd@chromium.org changed reviewers:
+ ddorwin@chromium.org, jrummell@chromium.org

[sandersd (OOO until July 31), 2016-03-03 02:31:57]

Description was changed from

==========
Do not change the case of titles in Utils.setResultInTitle().

This behavior causes mentions of the 'ended' event to immediately end
MSE tests (successfully). This is true even if the actual call was to
failTest().
==========

to

==========
Do not change the case of titles in Utils.setResultInTitle().

The previous behavior causes mentions of the 'ended' event to
immediately end MSE tests (successfully). This is true even if
the actual call was to Utils.failTest().
==========

[ddorwin, 2016-03-04 19:00:16]

Does this problem exist in other browser tests? We should consider more unique
names throughout, especially for "ENDED".

https://codereview.chromium.org/1755403002/diff/60001/chrome/browser/media/me...
File chrome/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/1755403002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_browsertest.cc:24: const char
MediaBrowserTest::kEnded[] = "ENDED";
Should we make this more unique as well? "TEST_ENDED/COMPLETE" or something?

https://codereview.chromium.org/1755403002/diff/60001/media/test/data/eme_pla...
File media/test/data/eme_player_js/utils.js (right):

https://codereview.chromium.org/1755403002/diff/60001/media/test/data/eme_pla...
media/test/data/eme_player_js/utils.js:184:
Utils.setResultInTitle(e.type.toUpperCase());
There is an "ended" event that would conflict with the "ENDED" constant!

Also, consider "EVENT_RECEIVED-e.type.toUpperCase()".

[sandersd (OOO until July 31), 2016-03-04 19:22:57]

Description was changed from

==========
Do not change the case of titles in Utils.setResultInTitle().

The previous behavior causes mentions of the 'ended' event to
immediately end MSE tests (successfully). This is true even if
the actual call was to Utils.failTest().
==========

to

==========
Do not change the case of titles in Utils.setResultInTitle().

The previous behavior causes mentions of the 'ended' event to
immediately end MSE tests (successfully). This is true even if
the actual call was to Utils.failTest().

BUG=592067
==========

[sandersd (OOO until July 31), 2016-03-04 19:26:00]

https://codereview.chromium.org/1755403002/diff/60001/chrome/browser/media/me...
File chrome/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/1755403002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_browsertest.cc:24: const char
MediaBrowserTest::kEnded[] = "ENDED";
On 2016/03/04 19:00:16, ddorwin wrote:
> Should we make this more unique as well? "TEST_ENDED/COMPLETE" or something?

We certainly should. I've filed a bug to track these changes.

https://codereview.chromium.org/1755403002/diff/60001/media/test/data/eme_pla...
File media/test/data/eme_player_js/utils.js (right):

https://codereview.chromium.org/1755403002/diff/60001/media/test/data/eme_pla...
media/test/data/eme_player_js/utils.js:184:
Utils.setResultInTitle(e.type.toUpperCase());
On 2016/03/04 19:00:16, ddorwin wrote:
> There is an "ended" event that would conflict with the "ENDED" constant!
> 
> Also, consider "EVENT_RECEIVED-e.type.toUpperCase()".

Indeed there is, but tests are explicitly relying on that. And also note that
adding a prefix without changing the constant makes no difference, since
titlewatcher doesn't require the string to be at the start.

In general, this CL assumes that installTitleEventHandler() is intended to end
the test, but any call to failTest() should not cause the test to succeed. Hence
the case difference between those two paths.

[ddorwin, 2016-03-04 20:14:07]

Do I understand correctly that this CL fixes the case where a test that set the
title to "waiting for the ended event" would cause the test to succeed early
because it gets upper cased and matches "ENDED"?

If so, LGTM, though there is clearly opportunity for improvement when fixing the
bug.

https://codereview.chromium.org/1755403002/diff/60001/chrome/browser/media/me...
File chrome/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/1755403002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_browsertest.cc:24: const char
MediaBrowserTest::kEnded[] = "ENDED";
On 2016/03/04 19:26:00, sandersd wrote:
> On 2016/03/04 19:00:16, ddorwin wrote:
> > Should we make this more unique as well? "TEST_ENDED/COMPLETE" or something?
> 
> We certainly should. I've filed a bug to track these changes.

FTR, https://bugs.chromium.org/p/chromium/issues/detail?id=592067

https://codereview.chromium.org/1755403002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_browsertest.cc:24: const char
MediaBrowserTest::kEnded[] = "ENDED";
Perhaps this should be kEndedEvent to make it clear that we are talking about a
JS event.

https://codereview.chromium.org/1755403002/diff/60001/media/test/data/eme_pla...
File media/test/data/eme_player_js/utils.js (right):

https://codereview.chromium.org/1755403002/diff/60001/media/test/data/eme_pla...
media/test/data/eme_player_js/utils.js:184:
Utils.setResultInTitle(e.type.toUpperCase());
On 2016/03/04 19:26:00, sandersd wrote:
> On 2016/03/04 19:00:16, ddorwin wrote:
> > There is an "ended" event that would conflict with the "ENDED" constant!
> > 
> > Also, consider "EVENT_RECEIVED-e.type.toUpperCase()".
> 
> Indeed there is, but tests are explicitly relying on that. And also note that
> adding a prefix without changing the constant makes no difference, since
> titlewatcher doesn't require the string to be at the start.

Hmm. Maybe all the expected strings should have spaces or something (e.g. "E N D
E D"). We can write a JS function to do that here.

> In general, this CL assumes that installTitleEventHandler() is intended to end
> the test, but any call to failTest() should not cause the test to succeed.
Hence
> the case difference between those two paths.

Ah, I originally thought "ENDED" was tests complete, but we're actually looking
for playback to have ended.

[sandersd (OOO until July 31), 2016-03-04 20:40:03]

> Do I understand correctly that this CL fixes the case where a test that set
the
> title to "waiting for the ended event" would cause the test to succeed early
> because it gets upper cased and matches "ENDED"?

That's approximately correct, but there is one specific case that prompted this
CL. In many of our tests, we have this JS:

  video.addEventListener('ended', Utils.failTest);

But what was happening is that this resulted in changing the title to "ended",
the type of the event itself, along with the message "video.ended" in the log.
Then Utils.setResultInTitle() was upper-casing it, resulting in the final title
"ENDED", which was indistinguishable from a success.

[sandersd (OOO until July 31), 2016-03-04 20:40:17]

https://codereview.chromium.org/1755403002/diff/60001/chrome/browser/media/me...
File chrome/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/1755403002/diff/60001/chrome/browser/media/me...
chrome/browser/media/media_browsertest.cc:24: const char
MediaBrowserTest::kEnded[] = "ENDED";
On 2016/03/04 20:14:06, ddorwin wrote:
> Perhaps this should be kEndedEvent to make it clear that we are talking about
a
> JS event.

I'm not certain that it is always used to mean that. It is for the specific
MSE+EME tests I was looking at, but there are others here I am less sure about.
I expect that any refactoring as requested in the bug will improve this.

https://codereview.chromium.org/1755403002/diff/60001/media/test/data/eme_pla...
File media/test/data/eme_player_js/utils.js (right):

https://codereview.chromium.org/1755403002/diff/60001/media/test/data/eme_pla...
media/test/data/eme_player_js/utils.js:184:
Utils.setResultInTitle(e.type.toUpperCase());
On 2016/03/04 20:14:06, ddorwin wrote:
> On 2016/03/04 19:26:00, sandersd wrote:
> > On 2016/03/04 19:00:16, ddorwin wrote:
> > > There is an "ended" event that would conflict with the "ENDED" constant!
> > > 
> > > Also, consider "EVENT_RECEIVED-e.type.toUpperCase()".
> > 
> > Indeed there is, but tests are explicitly relying on that. And also note
that
> > adding a prefix without changing the constant makes no difference, since
> > titlewatcher doesn't require the string to be at the start.
> 
> Hmm. Maybe all the expected strings should have spaces or something (e.g. "E N
D
> E D"). We can write a JS function to do that here.
> 
> > In general, this CL assumes that installTitleEventHandler() is intended to
end
> > the test, but any call to failTest() should not cause the test to succeed.
> Hence
> > the case difference between those two paths.
> 
> Ah, I originally thought "ENDED" was tests complete, but we're actually
looking
> for playback to have ended.

Acknowledged.

[ddorwin, 2016-03-04 21:30:40]

On 2016/03/04 20:40:03, sandersd wrote:
> > Do I understand correctly that this CL fixes the case where a test that set
> the
> > title to "waiting for the ended event" would cause the test to succeed early
> > because it gets upper cased and matches "ENDED"?
> 
> That's approximately correct, but there is one specific case that prompted
this
> CL. In many of our tests, we have this JS:
> 
>   video.addEventListener('ended', Utils.failTest);
> 
> But what was happening is that this resulted in changing the title to "ended",
> the type of the event itself, along with the message "video.ended" in the log.
> Then Utils.setResultInTitle() was upper-casing it, resulting in the final
title
> "ENDED", which was indistinguishable from a success.

Thanks. LGTM. Please add this specific example and other issues/ideas we
discussed in this CL to the bug so we don't forget them.

[sandersd (OOO until July 31), 2016-03-04 21:46:03]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2016-03-04 21:46:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1755403002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1755403002/60001

[commit-bot: I haz the power, 2016-03-04 23:03:34]

Description was changed from

==========
Do not change the case of titles in Utils.setResultInTitle().

The previous behavior causes mentions of the 'ended' event to
immediately end MSE tests (successfully). This is true even if
the actual call was to Utils.failTest().

BUG=592067
==========

to

==========
Do not change the case of titles in Utils.setResultInTitle().

The previous behavior causes mentions of the 'ended' event to
immediately end MSE tests (successfully). This is true even if
the actual call was to Utils.failTest().

BUG=592067
==========

[commit-bot: I haz the power, 2016-03-04 23:03:35]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-03-04 23:05:33]

Description was changed from

==========
Do not change the case of titles in Utils.setResultInTitle().

The previous behavior causes mentions of the 'ended' event to
immediately end MSE tests (successfully). This is true even if
the actual call was to Utils.failTest().

BUG=592067
==========

to

==========
Do not change the case of titles in Utils.setResultInTitle().

The previous behavior causes mentions of the 'ended' event to
immediately end MSE tests (successfully). This is true even if
the actual call was to Utils.failTest().

BUG=592067

Committed: https://crrev.com/6eb52c23c889bba614b089ca0952f419af27412e
Cr-Commit-Position: refs/heads/master@{#379382}
==========

[commit-bot: I haz the power, 2016-03-04 23:05:34]

Patchset 3 (id:??) landed as
https://crrev.com/6eb52c23c889bba614b089ca0952f419af27412e
Cr-Commit-Position: refs/heads/master@{#379382}