Add Origin Trial layout tests for IDL bindings

Add Origin Trial layout tests for IDL bindings

The existing layout tests for Origin Trials cover both various enabling
scenarios, as well as bindings for [OriginTrialEnabled].

As shown by bug 695123, there isn't comprehensive coverage for all the
bindings possibilities.

This CL adds a separate set of layout tests explicitly for bindings,
in preparation for fixing bug 695123.

BUG=695123
Review-Url: https://codereview.chromium.org/2861673007
Cr-Commit-Position: refs/heads/master@{#469862}
Committed: https://chromium.googlesource.com/chromium/src/+/fbd04b8d08a73374d9720de6c01ff36dd13428a1

[chasej, 2017-05-04 17:01:06]

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

[commit-bot: I haz the power, 2017-05-04 17:01:29]

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-05-04 17:41:31]

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

[commit-bot: I haz the power, 2017-05-04 17:41:32]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  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-05-04 20:03:56]

Description was changed from

==========
Add Origin Trial layout tests for IDL bindings

The existing layout tests for Origin Trials cover both various enabling
scenarios, as well as bindings for [OriginTrialEnabled].

As shown by bug 695123, there isn't comprehensive coverage for all the
bindings possibilities.

This CL adds a separate set of layout tests explicitly for bindings.

BUG=695123
==========

to

==========
Add Origin Trial layout tests for IDL bindings

The existing layout tests for Origin Trials cover both various enabling
scenarios, as well as bindings for [OriginTrialEnabled].

As shown by bug 695123, there isn't comprehensive coverage for all the
bindings possibilities.

This CL adds a separate set of layout tests explicitly for bindings,
in preparation for fixing bug 695123.

BUG=695123
==========

[chasej, 2017-05-04 20:04:27]

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

[chasej, 2017-05-04 20:04:28]

iclelland, please take a look.

[iclelland, 2017-05-05 14:11:34]

This looks great, very comprehensive. Just a couple of nits around making sure
the tests are clear, in case someone else wants to extend this someday.

Have you checked the generated files to see what's up with the windows compile
error?

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js
(right):

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:6:
expect_member = (name, get_value_func) => {
I like the use of the lambdas here -- can you document what this method is
expected to test? Reading through the code that calls it, it wasn't apparent
immediately that 'name' wasn't just a docstring, but is *also* used to test
whether the member is visible.

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:10:
'Member should return boolean value');
The member is required to return 'true', and not just any boolean, right? We
should either change the message to "Member should return true", or have this
method taken an expected value like expect_constant does, and then we can use
assert_equals instead.

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:26:
'Constant should return integer value');
Maybe change "integer" to "expected"; unless there's good reason to expect an
int in every case. (Or else maybe rename to "expect_numeric_constant", and
actually check that its type is Number -- ints aren't really special in JS)

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:461:
expect_failure_bindings = (description_suffix) => {
Nit: This should probably have a comment, for parity with all of the other
methods around it.

[chasej, 2017-05-05 15:29:29]

iclelland, please take another look.

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js
(right):

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:6:
expect_member = (name, get_value_func) => {
On 2017/05/05 14:11:34, iclelland wrote:
> I like the use of the lambdas here -- can you document what this method is
> expected to test? Reading through the code that calls it, it wasn't apparent
> immediately that 'name' wasn't just a docstring, but is *also* used to test
> whether the member is visible.

I documented what each expect function does. I also renamed to "member_name", so
hopefully that minimizes confusion.

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:10:
'Member should return boolean value');
On 2017/05/05 14:11:34, iclelland wrote:
> The member is required to return 'true', and not just any boolean, right? We
> should either change the message to "Member should return true", or have this
> method taken an expected value like expect_constant does, and then we can use
> assert_equals instead.

I prefer to leave as is.

The specific return value/type isn't really important. It's really testing that
the member exists and returns a real value (i.e. not undefined). For simplicity,
all the attributes/methods are defined as boolean. Obviously, "true" is better,
since undefined won't convert to true. I think adding an expected value is just
extra code for minimal value. For expect_constant, I think the expected value is
useful to distinguish different constants.

As for the message, a failed assertion will clearly state that true was
expected, but got false instead.

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:26:
'Constant should return integer value');
On 2017/05/05 14:11:34, iclelland wrote:
> Maybe change "integer" to "expected"; unless there's good reason to expect an
> int in every case. (Or else maybe rename to "expect_numeric_constant", and
> actually check that its type is Number -- ints aren't really special in JS)

Done. Changed to "expected". The type isn't really important to testing the
constant.

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:461:
expect_failure_bindings = (description_suffix) => {
On 2017/05/05 14:11:34, iclelland wrote:
> Nit: This should probably have a comment, for parity with all of the other
> methods around it.

Done.

[iclelland, 2017-05-05 15:37:27]

Thanks, this lgtm -- once the compile failures are fixed, obviously :)

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js
(right):

https://codereview.chromium.org/2861673007/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/origin_trials/resources/origintrials.js:10:
'Member should return boolean value');
On 2017/05/05 15:29:29, chasej wrote:
> On 2017/05/05 14:11:34, iclelland wrote:
> > The member is required to return 'true', and not just any boolean, right? We
> > should either change the message to "Member should return true", or have
this
> > method taken an expected value like expect_constant does, and then we can
use
> > assert_equals instead.
> 
> I prefer to leave as is.
> 
> The specific return value/type isn't really important. It's really testing
that
> the member exists and returns a real value (i.e. not undefined). For
simplicity,
> all the attributes/methods are defined as boolean. Obviously, "true" is
better,
> since undefined won't convert to true. I think adding an expected value is
just
> extra code for minimal value. For expect_constant, I think the expected value
is
> useful to distinguish different constants.
> 
> As for the message, a failed assertion will clearly state that true was
> expected, but got false instead.

Acknowledged. That makes sense.

[chasej, 2017-05-05 21:16:59]

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

[commit-bot: I haz the power, 2017-05-05 21:17:49]

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-05-05 23:20:45]

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

[commit-bot: I haz the power, 2017-05-05 23:20:46]

Dry run: This issue passed the CQ dry run.

[chasej, 2017-05-06 00:01:11]

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

[chasej, 2017-05-06 00:01:12]

jbroman, please take a look at third_party/WebKit/Source/core/testing/

[jbroman, 2017-05-06 00:22:16]

rs lgtm

[chasej, 2017-05-06 01:04:01]

The CQ bit was checked by chasej@chromium.org

[chasej, 2017-05-06 01:04:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/2861673007/#ps40001
(title: "Fix compile errors")

[commit-bot: I haz the power, 2017-05-06 01:04:39]

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-05-06 19:03:57]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494032641964950,
"parent_rev": "05fa41d9751a443a76dde0fe53cbfa927bb5d001", "commit_rev":
"fbd04b8d08a73374d9720de6c01ff36dd13428a1"}

[commit-bot: I haz the power, 2017-05-06 19:04:10]

Description was changed from

==========
Add Origin Trial layout tests for IDL bindings

The existing layout tests for Origin Trials cover both various enabling
scenarios, as well as bindings for [OriginTrialEnabled].

As shown by bug 695123, there isn't comprehensive coverage for all the
bindings possibilities.

This CL adds a separate set of layout tests explicitly for bindings,
in preparation for fixing bug 695123.

BUG=695123
==========

to

==========
Add Origin Trial layout tests for IDL bindings

The existing layout tests for Origin Trials cover both various enabling
scenarios, as well as bindings for [OriginTrialEnabled].

As shown by bug 695123, there isn't comprehensive coverage for all the
bindings possibilities.

This CL adds a separate set of layout tests explicitly for bindings,
in preparation for fixing bug 695123.

BUG=695123

Review-Url: https://codereview.chromium.org/2861673007
Cr-Commit-Position: refs/heads/master@{#469862}
Committed:
https://chromium.googlesource.com/chromium/src/+/fbd04b8d08a73374d9720de6c01f...
==========

[commit-bot: I haz the power, 2017-05-06 19:04:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/fbd04b8d08a73374d9720de6c01f...