JavaScript bindings for Mojo message validation

mojo/public/js/bindings/validation_unittests.js

Index: mojo/public/js/bindings/validation_unittests.js
diff --git a/mojo/public/js/bindings/validation_unittests.js b/mojo/public/js/bindings/validation_unittests.js
index ae276fbc1d44b8a2ec63dfeab1b521d300b12467..2720c8e2c11e780b7b3effb781b67823c9085532 100644
--- a/mojo/public/js/bindings/validation_unittests.js
+++ b/mojo/public/js/bindings/validation_unittests.js
@@ -162,7 +162,7 @@ define([
     return null;
   }
 
-  function getMessageTestFiles() {
+  function getMessageTestFiles(key) {
     var sourceRoot = file.getSourceRootDirectory();
     expect(sourceRoot).not.toBeNull();
 
@@ -172,17 +172,14 @@ define([
     expect(testFiles).not.toBeNull();
     expect(testFiles.length).toBeGreaterThan(0);
 
-   // The ".data" pathnames with the extension removed.
-   var testPathNames = testFiles.filter(function(s) {
-     return s.substr(-5) == ".data";
-   }).map(function(s) {
-     return testDir + s.slice(0, -5);
-   });
-
-   // For now, just checking the message header tests.
-   return testPathNames.filter(function(s) {
-     return s.indexOf("_msghdr_") != -1;
-   });
+    // The matching ".data" pathnames with the extension removed.
+    return testFiles.filter(function(s) {
+      return s.substr(-5) == ".data";
+    }).map(function(s) {
+      return testDir + s.slice(0, -5);
+    }).filter(function(s) {
+      return s.indexOf(key) != -1;
+    });
   }
 
   function readTestMessage(filename) {
@@ -197,21 +194,49 @@ define([
     return contents.trim();
   }
 
-  function testValidateMessageHeader() {
-    var testFiles = getMessageTestFiles();
+  function testMessageValidation(key, filters) {
+    var testFiles = getMessageTestFiles(key);
     expect(testFiles.length).toBeGreaterThan(0);
 
+    var noError = validator.validationError.NONE;
     for (var i = 0; i < testFiles.length; i++) {
+      // TODO(hansmuller): Temporarily skipping array pointer overflow tests.
+      if (testFiles[i].indexOf("overflow") != -1)

[yzshen1, 2014/08/14 00:06:32]

It may be nice to output a warning with the file name saying that the test is
skipped.

[hansmuller, 2014/08/14 00:57:57]

Good point, I've done that.

+        continue;
+
       var testMessage = readTestMessage(testFiles[i]);
-      // TODO(hansmuller): add the message handles.
-      var message = new codec.Message(testMessage.buffer);
-      var actualResult = new validator.Validator(message).validateMessage();
+      var handles = new Array(testMessage.handleCount);
+      var message =  new codec.Message(testMessage.buffer, handles);

[yzshen1, 2014/08/14 00:06:32]

extra space after =

[hansmuller, 2014/08/14 00:57:57]

Done.

+      var messageValidator = new validator.Validator(message);
+
+      var err = messageValidator.validateMessageHeader();

[yzshen1, 2014/08/14 00:06:32]

Something to think about:
It seems a little inconsistent:
messageValidator.validateMessageHeader() and
XXX.validateXXXXRequest(messageValidator).

Maybe we could have a ValidationContext object, and then do:
validateMessageHeader(validationContext)
XXX.validateXXXRequest(validationContext)
?

[hansmuller, 2014/08/14 00:57:56]

Acknowledged.

+      for (var j = 0; err === noError && j < filters.length; ++j)
+        err = filters[j](messageValidator);
+
+      var actualResult = (err === noError) ? "PASS" : err;
       var expectedResult = readTestExpected(testFiles[i]);
       expect(actualResult).toEqual(expectedResult);

[yzshen1, 2014/08/14 00:06:32]

It seems nice if we print out the file name on failure.

[hansmuller, 2014/08/14 00:57:57]

I agree. The simple JS expect framework doesn't appear to provide a way to
inject a failure message, so I've added an extra (trivial) test as well.

     }
   }
 
-  testValidateMessageHeader();
+  function testConformanceMessageValidation() {
+    testMessageValidation("conformance_", [
+      testInterface.validateConformanceTestInterfaceRequest,
+      testInterface.validateConformanceTestInterfaceResponse

[yzshen1, 2014/08/14 00:06:32]

All the conformance test cases are for ConformanceTestInterface*Request*.

It is incorrect to include the Response validator here.

(And the goal is to test our validation logic, not the ConformanceTestInterface
itself. We don't have to test its both Validate functions. Besides, the Response
validator is empty anyway.)

[hansmuller, 2014/08/14 00:57:56]

Done.

+    ]);
+  }
+
+  function testIntegrationMessageValidation() {
+    testMessageValidation("integration_", [

[yzshen1, 2014/08/14 00:06:32]

The goal of the integration tests is to make sure validators are hooked up with
the router correctly. This test is not doing that. Maybe we can do the
integration tests in the next CL, when the router actually uses the payload
validators?

+      testInterface.validateIntegrationTestInterface1Request,

[yzshen1, 2014/08/14 00:06:32]

The tests are designed for a message pipe endpoint receiving Interface1 request
and Interface2 response.

It doesn't make sense to add all the four payload validators.

[hansmuller, 2014/08/14 00:57:57]

I hadn't completely understood the scope of message names, thanks for clearing
that up.  I've just included the two filter methods you suggested.

+      testInterface.validateIntegrationTestInterface1Response,
+      testInterface.validateIntegrationTestInterface2Request,
+      testInterface.validateIntegrationTestInterface2Response
+    ]);
+  }
+
+  testConformanceMessageValidation();
+  testIntegrationMessageValidation();
   expect(checkTestMessageParser()).toBeNull();
   this.result = "PASS";
 });