Mojo JS validation unit test should check Connection et al

Mojo JS validation unit test should check Connection et al

Replaced the test for validation of requests and responses
with one that relies on the Connection et al classes to set
up the validators and deliver messages.

BUG=413303
Committed: https://crrev.com/d593dea8e8578f554e80c9254d77259ff0d6e122
Cr-Commit-Position: refs/heads/master@{#294924}

[hansmuller, 2014-09-12 23:02:21]

hansmuller@chromium.org changed reviewers:
+ yzshen@chromium.org

[hansmuller, 2014-09-12 23:03:31]

PTAL when you get a chance.

[yzshen1, 2014-09-15 07:08:41]

https://codereview.chromium.org/551293004/diff/1/mojo/mojo_public_tests.gypi
File mojo/mojo_public_tests.gypi (right):

https://codereview.chromium.org/551293004/diff/1/mojo/mojo_public_tests.gypi#...
mojo/mojo_public_tests.gypi:220: 'mojo_environment_standalone',
Please order them alphabetically.

https://codereview.chromium.org/551293004/diff/1/mojo/public/js/bindings/vali...
File mojo/public/js/bindings/validation_unittests.js (right):

https://codereview.chromium.org/551293004/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validation_unittests.js:216: var actualResult = (err ===
noError) ? "PASS" : err;
wrong indent.

https://codereview.chromium.org/551293004/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validation_unittests.js:287: validationError = err;
nit: wrong indent.

https://codereview.chromium.org/551293004/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validation_unittests.js:304: var waitToReadMore =
connector.Connector.prototype.waitToReadMore_;
[optional, something that may worth considering]
It seems a little fragile to modify the internal logic of Connector from here.
An alternative is to have a "testing" mode built into Connector. And here we
just set the Connector to testing mode. (But we need to make sure that the
change doesn't have any negative effect on non-testing mode.) What do you think?

[hansmuller, 2014-09-15 16:25:26]

Thanks for the feedback, I've made the suggested changes.

https://codereview.chromium.org/551293004/diff/1/mojo/mojo_public_tests.gypi
File mojo/mojo_public_tests.gypi (right):

https://codereview.chromium.org/551293004/diff/1/mojo/mojo_public_tests.gypi#...
mojo/mojo_public_tests.gypi:220: 'mojo_environment_standalone',
On 2014/09/15 07:08:40, yzshen1 wrote:
> Please order them alphabetically.

Done.

https://codereview.chromium.org/551293004/diff/1/mojo/public/js/bindings/vali...
File mojo/public/js/bindings/validation_unittests.js (right):

https://codereview.chromium.org/551293004/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validation_unittests.js:216: var actualResult = (err ===
noError) ? "PASS" : err;
On 2014/09/15 07:08:40, yzshen1 wrote:
> wrong indent.

Done.

https://codereview.chromium.org/551293004/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validation_unittests.js:287: validationError = err;
On 2014/09/15 07:08:40, yzshen1 wrote:
> nit: wrong indent.

Done.

https://codereview.chromium.org/551293004/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validation_unittests.js:304: var waitToReadMore =
connector.Connector.prototype.waitToReadMore_;
On 2014/09/15 07:08:40, yzshen1 wrote:
> [optional, something that may worth considering]
> It seems a little fragile to modify the internal logic of Connector from here.
> An alternative is to have a "testing" mode built into Connector. And here we
> just set the Connector to testing mode. (But we need to make sure that the
> change doesn't have any negative effect on non-testing mode.) What do you
think?

I agree and I have to admit that what I had been doing was a bit of a hack. 
I've created "Test" sublasses of Connection, Router, and Connector, that enable
testing. They don't impose any overhead on ordinary code.

[yzshen1, 2014-09-15 17:32:19]

Thanks, Hans. LGTM with one nit.

https://codereview.chromium.org/551293004/diff/1/mojo/public/js/bindings/vali...
File mojo/public/js/bindings/validation_unittests.js (right):

https://codereview.chromium.org/551293004/diff/1/mojo/public/js/bindings/vali...
mojo/public/js/bindings/validation_unittests.js:304: var waitToReadMore =
connector.Connector.prototype.waitToReadMore_;
I like your new approach!

https://codereview.chromium.org/551293004/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/router.js (right):

https://codereview.chromium.org/551293004/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/router.js:127: function(message, error) {
formatting nit:
I guess we want line 127 to have 4-space indent, and leave 128,129 as they are.
(But I am not a JS expert. :))

[hansmuller, 2014-09-15 18:07:34]

I've updated the indents. Will commit!

https://codereview.chromium.org/551293004/diff/20001/mojo/public/js/bindings/...
File mojo/public/js/bindings/router.js (right):

https://codereview.chromium.org/551293004/diff/20001/mojo/public/js/bindings/...
mojo/public/js/bindings/router.js:127: function(message, error) {
On 2014/09/15 17:32:19, yzshen1 wrote:
> formatting nit:
> I guess we want line 127 to have 4-space indent, and leave 128,129 as they
are.
> (But I am not a JS expert. :))

As far as I can tell from the style guide, either of the following variations
would be OK:

  TestRouter.prototype.handleInvalidIncomingMessage_ =
      function(message, error) {
        this.validationErrorHandler(error);
      };

  TestRouter.prototype.handleInvalidIncomingMessage_ = function(
      message, error) {
    this.validationErrorHandler(error);
  };

I think the first one, which is what you're suggesting, is more readable, so
I'll use that.

[hansmuller, 2014-09-15 19:16:08]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-09-15 19:16:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551293004/40001

[commit-bot: I haz the power, 2014-09-15 20:11:57]

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

[commit-bot: I haz the power, 2014-09-15 20:11:58]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hansmuller, 2014-09-15 20:39:07]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-09-15 20:40:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551293004/40001

[commit-bot: I haz the power, 2014-09-15 21:00:13]

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

[commit-bot: I haz the power, 2014-09-15 21:00:14]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hansmuller, 2014-09-15 21:43:58]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-09-15 22:03:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551293004/60001

[hansmuller, 2014-09-15 22:05:06]

The CQ bit was unchecked by hansmuller@chromium.org

[hansmuller, 2014-09-15 22:05:06]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-09-15 22:39:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551293004/60001

[commit-bot: I haz the power, 2014-09-15 23:41:03]

Committed patchset #4 (id:60001) as 18e8959d63b20f53833bdb9edbc688cd37d2e4f3

[commit-bot: I haz the power, 2014-09-15 23:48:29]

Patchset 4 (id:??) landed as
https://crrev.com/d593dea8e8578f554e80c9254d77259ff0d6e122
Cr-Commit-Position: refs/heads/master@{#294924}