Use asserts in origin trials layout tests

third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js

Index: third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js
diff --git a/third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js b/third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js
index af07d129e656088b45d03f09060990406a164f63..dd48b1784d549c052c69409a6c8200f566b8c129 100644
--- a/third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js
+++ b/third_party/WebKit/LayoutTests/http/tests/resources/origin-trials-helper.js
@@ -2,30 +2,37 @@
 // LayoutTests that are checking members exposed to script by origin trials.
 //
 // The current available methods are:
-// get_interface_names:
-//   Report on the existence of the given interface names on the global object,
-//   listing all the properties found for each interface. The properties can be
-//   filtered by providing a list of desired property names. As well, it can
-//   report on properties of the global object itself, by giving 'global' as one
-//   of the interface names.
+// check_properties:
+//   Tests for the existence of the given property names, on the given interface
+//   names, on the global object. As well, it can test for properties of the
+//   global object itself, by giving 'global' as the interface name.
 // Example:
-//   OriginTrialsHelper.get_interface_names(
+//   OriginTrialsHelper.check_properties(
 //     this,
-//     ['ForeignFetchEvent', 'InstallEvent', 'global'],
-//     {'InstallEvent':['registerForeignFetch'], 'global':['onforeignfetch']}));
+//     {'InstallEvent':['registerForeignFetch'], 'global':['onforeignfetch']});
 //
-// check_interfaces_in_sw:
-//   Collects the results of calling get_interface_names() in a service worker.
-//   Use in a promise test to output the results.
+// check_properties_missing:
+//   Tests that the given property names do not exist on the global object. That
+//   is, tests for the opposite of check_properties().
 // Example:
-//   promise_test(t => {
-//     var script = 'path/to/script/calling/get_interface_names()'
-//     var scope = 'matching scope'
-//     return OriginTrialsHelper.check_interfaces_in_sw(t, script, scope)
-//        .then(message => {
-//          console.log('Interfaces in Service Worker - origin trial enabled.\n'
-//                      + message);
-//        });
+//   OriginTrialsHelper.check_properties_missing(
+//     this,
+//     {'InstallEvent':['registerForeignFetch'], 'global':['onforeignfetch']});
+//
+// check_interfaces:

[iclelland, 2017/02/24 18:38:42]

Should check_interfaces also test that the properties found are actually
interface objects?

[chasej, 2017/02/24 20:04:07]

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.

+//   Tests for the existence of the given interface names, on the global object.
+// Example:
+//   OriginTrialsHelper.check_interfaces(
+//     this,
+//     ['USBAlternateInterface', 'USBConfiguration']);
+//
+// check_interfaces_missing:
+//   Tests that the given interface names do not exist on the global object.
+//   That is, tests for the opposite of check_interfaces().
+// Example:
+//   OriginTrialsHelper.check_interfaces_missing(
+//     this,
+//     ['USBAlternateInterface', 'USBConfiguration']);
 //
 // add_token:
 //   Adds a trial token to the document, to enable a trial via script
@@ -35,100 +42,62 @@
 
 var OriginTrialsHelper = (function() {
   return {
-    get_interface_names:
-        (global_object, interface_names,
-         opt_property_filters) => {
-          var property_filters = opt_property_filters || {};
-          var result = [];
-          function collect_property_info(object, property_name, output) {
-            var descriptor =
-                Object.getOwnPropertyDescriptor(object, property_name);
-            if ('value' in descriptor) {
-              if (typeof descriptor.value === 'function') {
-                output.push('    method ' + property_name);
-              } else {
-                output.push('    attribute ' + property_name);
-              }
-            } else {
-              if (descriptor.get) {
-                output.push('    getter ' + property_name);
-              }
-              if (descriptor.set) {
-                output.push('    setter ' + property_name);
-              }
-            }
+    check_properties_impl: (global_object, property_filters, should_exist) => {
+      let interface_names = Object.getOwnPropertyNames(property_filters).sort();
+      interface_names.forEach(function(interface_name) {
+        let use_global = false;
+        let interface_object;
+        if (interface_name === 'global') {
+          use_global = true;
+          interface_object = global_object;
+        } else {
+          interface_object = global_object[interface_name];
+        }
+        assert_true(interface_object !== undefined, 'Interface ' + interface_name + ' not found on provided object');

[iclelland, 2017/02/24 18:38:42]

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.

[chasej, 2017/02/24 20:04:07]

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.

+        if (interface_object === undefined) {

[iclelland, 2017/02/24 18:38:42]

Is this test necessary, if assert_true will throw an exception earlier?

[chasej, 2017/02/24 20:04:07]

No, removed.

+          return;
+        }
+        let interface_prototype = use_global ? interface_object : interface_object.prototype;

[iclelland, 2017/02/24 18:38:42]

Why not just have the foreach loop above define interface_prototype directly?
(interface_object isn't used below this point)

[chasej, 2017/02/24 20:04:07]

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.

+        property_filters[interface_name].forEach(function(property_name) {
+          if (should_exist) {
+            assert_own_property(interface_prototype, property_name, interface_name);
+          } else {
+            assert_false(interface_prototype.hasOwnProperty(property_name),

[iclelland, 2017/02/24 18:38:42]

You can use assert_not_exists for this, I think.

[chasej, 2017/02/24 20:04:07]

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?

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

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);

?

+                'Property ' + property_name + ' exists on ' + interface_name);
           }
-          interface_names.sort();
-          interface_names.forEach(function(interface_name) {
-            var use_global = false;
-            var interface_object;
-            if (interface_name === 'global') {
-              use_global = true;
-              interface_object = global_object;
-            } else {
-              interface_object = global_object[interface_name];
-            }
-            if (interface_object === undefined) {
-              return;
-            }
-            var interface_prototype;
-            var display_name;
-            if (use_global) {
-              interface_prototype = interface_object;
-              display_name = 'global object';
-            } else {
-              interface_prototype = interface_object.prototype;
-              display_name = 'interface ' + interface_name;
-            }
-            result.push(display_name);
-            var property_names =
-                Object.getOwnPropertyNames(interface_prototype);
-            var match_property_names = property_filters[interface_name];
-            if (match_property_names) {
-              property_names = property_names.filter(
-                  name => { return match_property_names.indexOf(name) >= 0; });
-            }
-            var property_strings = [];
-            property_names.forEach(function(property_name) {
-              collect_property_info(
-                  interface_prototype, property_name, property_strings);
-            });
-            result.push.apply(result, property_strings.sort());
-          });
-          return result.join('\n');
-        },
+        });
+      });
+    },
+
+    check_properties: (global_object, property_filters) => {
+      OriginTrialsHelper.check_properties_impl(global_object, property_filters, true);
+    },
+
+    check_properties_missing: (global_object, property_filters) => {
+      OriginTrialsHelper.check_properties_impl(global_object, property_filters, false);
+    },
+
+    check_interfaces_impl: (global_object, interface_names, should_exist) => {
+      interface_names.sort();
+      interface_names.forEach(function(interface_name) {
+        assert_equals(global_object[interface_name] !== undefined, should_exist,
+          'Interface ' + interface_name + ' exists on provided object');

[iclelland, 2017/02/24 18:38:42]

When should_exist is true (and the interface doesn't exist), does this message
still make sense?

[chasej, 2017/02/24 20:04:07]

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]

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

+      });
+    },
+
+    check_interfaces: (global_object, interface_names) => {
+      OriginTrialsHelper.check_interfaces_impl(global_object, interface_names, true);
+    },
 
-         check_interfaces_in_sw:
-             (t, script, scope) => {
-               var worker;
-               var message;
-               var registration;
-               return service_worker_unregister_and_register(t, script, scope)
-                   .then(reg => {
-                     registration = reg;
-                     worker = registration.installing;
-                     return wait_for_state(t, worker, 'activated');
-                   })
-                   .then(_ => {
-                     var saw_message = new Promise(resolve => {
-                       navigator.serviceWorker.onmessage =
-                           e => { resolve(e.data); };
-                     });
-                     worker.postMessage('');
-                     return saw_message;
-                   })
-                   .then(msg => {
-                     message = msg;
-                     return registration.unregister();
-                   })
-                   .then(_ => { return message; });
-             },
+    check_interfaces_missing: (global_object, interface_names) => {
+      OriginTrialsHelper.check_interfaces_impl(global_object, interface_names, false);
+    },
 
-         add_token: (token_string) => {
-           var tokenElement = document.createElement('meta');
-           tokenElement.httpEquiv = 'origin-trial';
-           tokenElement.content = token_string;
-           document.head.appendChild(tokenElement);
-         }
+    add_token: (token_string) => {
+      var tokenElement = document.createElement('meta');
+      tokenElement.httpEquiv = 'origin-trial';
+      tokenElement.content = token_string;
+      document.head.appendChild(tokenElement);
+    }
   }
 })();