Use asserts in origin trials layout tests

Use asserts in origin trials layout tests

The layout tests for exposure of interfaces were using console messages
with comparisons against -expected.txt files. Although they were using
testharness.js, the tests were not using asserts.

This CL changes the tests to use asserts, and remove the -expected.txt
files wherever possible.

Discussion on console messages in testharness tests can be found here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Ob2lq14KEs

BUG=687492
Review-Url: https://codereview.chromium.org/2712843005
Cr-Commit-Position: refs/heads/master@{#453063}
Committed: https://chromium.googlesource.com/chromium/src/+/2a99dd6adb183a1a501035ccd9777e189087de82

[chasej, 2017-02-23 22:02:05]

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

[commit-bot: I haz the power, 2017-02-23 22:03:38]

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, 2017-02-23 23:17:35]

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

[commit-bot: I haz the power, 2017-02-23 23:17:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chasej, 2017-02-24 05:50:01]

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

[commit-bot: I haz the power, 2017-02-24 05:50:18]

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, 2017-02-24 06:50:04]

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

[commit-bot: I haz the power, 2017-02-24 06:50:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chasej, 2017-02-24 15:18:28]

Description was changed from

==========
Use asserts in origin trials layout tests

BUG=687492
==========

to

==========
Use asserts in origin trials layout tests

The layout tests for exposure of interfaces were using console messages
with comparisons against -expected.txt files. Although they were using
testharness.js, the tests were not using asserts.

This CL changes the tests to use asserts, and remove the -expected.txt
files wherever possible.

Discussion on console messages in testharness tests can be found here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Ob2lq14KEs

BUG=687492
==========

[chasej, 2017-02-24 15:19:05]

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

[commit-bot: I haz the power, 2017-02-24 15:19:14]

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

[chasej, 2017-02-24 15:20:00]

chasej@chromium.org changed reviewers:
+ iclelland@chromium.org

[chasej, 2017-02-24 15:20:01]

iclelland, please take a look.

[commit-bot: I haz the power, 2017-02-24 16:23:11]

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

[commit-bot: I haz the power, 2017-02-24 16:23:11]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-02-24 18:38:43]

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js
(right):

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:22:
// check_interfaces:
Should check_interfaces also test that the properties found are actually
interface objects?

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:56:
assert_true(interface_object !== undefined, 'Interface ' + interface_name + '
not found on provided object');
This message seems wrong -- shouldn't it just tell the caller that the interface
wasn't found, or that it wasn't found on the global scope? "On the provided
object" might be misleading.

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:57:
if (interface_object === undefined) {
Is this test necessary, if assert_true will throw an exception earlier?

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:60:
let interface_prototype = use_global ? interface_object :
interface_object.prototype;
Why not just have the foreach loop above define interface_prototype directly?
(interface_object isn't used below this point)

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:65:
assert_false(interface_prototype.hasOwnProperty(property_name),
You can use assert_not_exists for this, I think.

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:84:
'Interface ' + interface_name + ' exists on provided object');
When should_exist is true (and the interface doesn't exist), does this message
still make sense?

[chasej, 2017-02-24 20:04:08]

Thanks for the comments. See below.

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js
(right):

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:22:
// check_interfaces:
On 2017/02/24 18:38:42, iclelland wrote:
> Should check_interfaces also test that the properties found are actually
> interface objects?

I intentionally simplified to just check for interfaces. The goal is to
avoid duplicating more comprehensive tests that I expect each feature
to have. Also, to make these tests less brittle if there are unrelated
changes to the interface (as far as the origin trial is concerned).

The idea is to write these tests just for the locations where the 
[OriginTrialEnabled] attribute is applied. The tests are for the manual
bindings code in ConditionalFeatures*, not for the [OriginTrialEnabled]
attribute, and so on.

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:56:
assert_true(interface_object !== undefined, 'Interface ' + interface_name + '
not found on provided object');
On 2017/02/24 18:38:42, iclelland wrote:
> This message seems wrong -- shouldn't it just tell the caller that the
interface
> wasn't found, or that it wasn't found on the global scope? "On the provided
> object" might be misleading.

The |global_object| could be anything, as far as this function knows, so
it doesn't seem right to say "global scope". I like the simpler
suggestion of "interface not found". The last bit of the message likely
doesn't add much helpful info.

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:57:
if (interface_object === undefined) {
On 2017/02/24 18:38:42, iclelland wrote:
> Is this test necessary, if assert_true will throw an exception earlier?

No, removed.

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:60:
let interface_prototype = use_global ? interface_object :
interface_object.prototype;
On 2017/02/24 18:38:42, iclelland wrote:
> Why not just have the foreach loop above define interface_prototype directly?
> (interface_object isn't used below this point)

I had separate steps to allow the assert on the interface existing. That
did seem like a lot of code though for what was actually happening. 
You're correct that I can simplify and still get the desired asserts.

Done.

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:65:
assert_false(interface_prototype.hasOwnProperty(property_name),
On 2017/02/24 18:38:42, iclelland wrote:
> You can use assert_not_exists for this, I think.

The documentation for testharness.js says that assert_not_exists is
deprecated. When I searched the layout tests, I only found a few
other usages. That suggested that maybe should not be using that
assert, and I didn't want to add to the problem. Although, I didn't\
find a reason for the deprecation, or suggested alternative.

Thoughts?

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:84:
'Interface ' + interface_name + ' exists on provided object');
On 2017/02/24 18:38:42, iclelland wrote:
> When should_exist is true (and the interface doesn't exist), does this message
> still make sense?

I think makes sense. The end result in the output is something like:
"Interface ForeignFetchEvent exists on provided object expected true but got
false"

That is, for assert_equals, it outputs the description, followed by
the expected and actual values.

[iclelland, 2017-02-24 20:23:08]

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js
(right):

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:65:
assert_false(interface_prototype.hasOwnProperty(property_name),
On 2017/02/24 20:04:07, chasej wrote:
> On 2017/02/24 18:38:42, iclelland wrote:
> > You can use assert_not_exists for this, I think.
> 
> The documentation for testharness.js says that assert_not_exists is
> deprecated. When I searched the layout tests, I only found a few
> other usages. That suggested that maybe should not be using that
> assert, and I didn't want to add to the problem. Although, I didn't\
> find a reason for the deprecation, or suggested alternative.
> 
> Thoughts?

I didn't see anything to that effect in the testharness.js file; I was actually
going to suggest using assert_exists and assert_not_exists for symmetry. It
seems odd that the two tests aren't mirrors of each other.

assert_own_property and assert_not_exists just test Object.hasOwnProperty -- can
you just replace the whole if block with something like

assert_equals(interface_prototype.hasOwnProperty(property_name), should_exist,
    'Property ' + property_name + ' exists on ' + interface_name);

?

https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:84:
'Interface ' + interface_name + ' exists on provided object');
On 2017/02/24 20:04:07, chasej wrote:
> On 2017/02/24 18:38:42, iclelland wrote:
> > When should_exist is true (and the interface doesn't exist), does this
message
> > still make sense?
> 
> I think makes sense. The end result in the output is something like:
> "Interface ForeignFetchEvent exists on provided object expected true but got
> false"
> 
> That is, for assert_equals, it outputs the description, followed by
> the expected and actual values.

Ahh, I forgot about the "Expected X Got Y" -- that makes sense, thanks.

https://codereview.chromium.org/2712843005/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js
(right):

https://codereview.chromium.org/2712843005/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:80:
assert_equals(global_object[interface_name] !== undefined, should_exist,
Is it necessary for this to test obj[name] === undefined, as opposed to using
obj.hasOwnProperty(name), like  check_properties_impl does?

[chasej, 2017-02-25 02:39:42]

On 2017/02/24 20:23:08, iclelland wrote:
>
https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js
> (right):
> 
>
https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:65:
> assert_false(interface_prototype.hasOwnProperty(property_name),
> On 2017/02/24 20:04:07, chasej wrote:
> > On 2017/02/24 18:38:42, iclelland wrote:
> > > You can use assert_not_exists for this, I think.
> > 
> > The documentation for testharness.js says that assert_not_exists is
> > deprecated. When I searched the layout tests, I only found a few
> > other usages. That suggested that maybe should not be using that
> > assert, and I didn't want to add to the problem. Although, I didn't\
> > find a reason for the deprecation, or suggested alternative.
> > 
> > Thoughts?
> 
> I didn't see anything to that effect in the testharness.js file; I was
actually
> going to suggest using assert_exists and assert_not_exists for symmetry. It
> seems odd that the two tests aren't mirrors of each other.
> 
> assert_own_property and assert_not_exists just test Object.hasOwnProperty --
can
> you just replace the whole if block with something like
> 
> assert_equals(interface_prototype.hasOwnProperty(property_name), should_exist,
>     'Property ' + property_name + ' exists on ' + interface_name);
> 
> ?
Agreed on having the two tests being symmetric. Changed to use hasOwnProperty()
as suggested.


>
https://codereview.chromium.org/2712843005/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:80:
> assert_equals(global_object[interface_name] !== undefined, should_exist,
> Is it necessary for this to test obj[name] === undefined, as opposed to using
> obj.hasOwnProperty(name), like  check_properties_impl does?
Changed to use hasOwnProperty().

(Offline, I mentioned some odd behaviour in the tests, but that was a bad edit
on my part.)

[iclelland, 2017-02-25 03:02:34]

On 2017/02/25 02:39:42, chasej wrote:
> On 2017/02/24 20:23:08, iclelland wrote:
> >
>
https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
> > File
> third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2712843005/diff/40001/third_party/WebKit/Layo...
> >
>
third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js:65:
> > assert_false(interface_prototype.hasOwnProperty(property_name),
> > On 2017/02/24 20:04:07, chasej wrote:
> > > On 2017/02/24 18:38:42, iclelland wrote:
> > > > You can use assert_not_exists for this, I think.
> > > 
> > > The documentation for testharness.js says that assert_not_exists is
> > > deprecated. When I searched the layout tests, I only found a few
> > > other usages. That suggested that maybe should not be using that
> > > assert, and I didn't want to add to the problem. Although, I didn't\
> > > find a reason for the deprecation, or suggested alternative.
> > > 
> > > Thoughts?
> > 
> > I didn't see anything to that effect in the testharness.js file; I was
> actually
> > going to suggest using assert_exists and assert_not_exists for symmetry. It
> > seems odd that the two tests aren't mirrors of each other.
> > 
> > assert_own_property and assert_not_exists just test Object.hasOwnProperty --
> can
> > you just replace the whole if block with something like
> > 
> > assert_equals(interface_prototype.hasOwnProperty(property_name),
should_exist,
> >     'Property ' + property_name + ' exists on ' + interface_name);
> > 
> > ?
> Agreed on having the two tests being symmetric. Changed to use
hasOwnProperty()
> as suggested.

+1+ for symmetry -- this LGTM, thanks!

[chasej, 2017-02-25 03:09:05]

The CQ bit was checked by chasej@chromium.org

[commit-bot: I haz the power, 2017-02-25 03:09:17]

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-02-25 04:11:40]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487992145717980,
"parent_rev": "6761779daf843b5afefd405fc3761e961d04313c", "commit_rev":
"2a99dd6adb183a1a501035ccd9777e189087de82"}

[commit-bot: I haz the power, 2017-02-25 04:12:38]

Description was changed from

==========
Use asserts in origin trials layout tests

The layout tests for exposure of interfaces were using console messages
with comparisons against -expected.txt files. Although they were using
testharness.js, the tests were not using asserts.

This CL changes the tests to use asserts, and remove the -expected.txt
files wherever possible.

Discussion on console messages in testharness tests can be found here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Ob2lq14KEs

BUG=687492
==========

to

==========
Use asserts in origin trials layout tests

The layout tests for exposure of interfaces were using console messages
with comparisons against -expected.txt files. Although they were using
testharness.js, the tests were not using asserts.

This CL changes the tests to use asserts, and remove the -expected.txt
files wherever possible.

Discussion on console messages in testharness tests can be found here:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Ob2lq14KEs

BUG=687492

Review-Url: https://codereview.chromium.org/2712843005
Cr-Commit-Position: refs/heads/master@{#453063}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a99dd6adb183a1a501035ccd977...
==========

[commit-bot: I haz the power, 2017-02-25 04:12:40]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2a99dd6adb183a1a501035ccd977...