Re: [encrypted-media-tests] Initial migration of a subset of Google tests (#3324)

The following comments are from Alexey in our Quality group:

1. Code questions:
 1. Playback-temporary.js
  1. Was [EventWatcher](https://github.com/w3c/testharness.js/blob/master/docs/api.md) considered for checking event sequence?  Probably because of a few noisy events test can’t be converted right away, but it is still worth consideration. 
  1. There are three bool variables for listening only to the first instance of events: _`allKeysUsableEvent`, _`playingEvent`, _`timeupdateEvent`. I don’t see any value checking of _`playingEvent`. What if this event will be fired twice? We will set it to true twice and push two ‘playing’ to _event, right? If this is an expected behavior? 
  1. There are a few assert_any calls. According to [testharness.js ](https://github.com/w3c/testharness.js/blob/master/docs/api.md): “tests with multiple allowed pass conditions are bad practice unless the spec specifically allows multiple behaviours.” If I understand the code correctly, they can be changed to `assert_in_array`.
  1. In `onMessage `function. This function is not clear to me. What will happen if event.messageType is `individualization-request`? Seems like we won’t push `license-request` to the events array and later the test will fail with failed assert_array_equals in onClosed. If that is the case, then should we fail the test immediately in case of getting `individualization-request`? Also if it is possible, that we will receive both events(and if that behavior is correct), then will we push ‘licence-response’ twice? And why do we push event.messageType instead of explicitly writing ‘licence-request’ if that is the only value we can push?
 1. Utils.js
  1. waitForEventAndRunStep function. According to code this function is supposed to work without stepTest parameter. This contradicts with function name (there won’t be step_func) and I didn’t see any usage without stepTest parameter.
  1. forceTestFailureFromPromise function. We are forcing timeout failure in case of several promise rejections (transformed exceptions), because these rejection won’t fail the test. I am not sure, that this is a good approach. Should we consider using [Promise Tests](https://github.com/w3c/testharness.js/blob/master/docs/api.md)(“The test completes when the returned promise resolves. The test fails if the returned promise rejects.”)? It will probably require more changes in tests, but it is still worth consideration. Also we probably don’t need to call test.done() after forcing timeout failure. If we keep it like that, may we try to change force_timeout to assert_uncreached?
1.  Style: I understand, that since some parts were taken from open source repos the style between different files may be different, but there are some inconsistencies within single files.
 1. Unification of arguments indentation. For example in “playback-temporary.js” I can see “`( event.target, _mediaKeySession );`”, “`( event.type, 'message');`”, “`(_mediaKeys);`”, “`(   config.initDataType || event.initDataType,`”
 1. Code duplication:  I see at least 3 times this part used:  `assert_equals(event.target, _video); assert_true(event instanceof window.MediaEncryptedEvent); assert_equals(event.type, 'encrypted');` this part can be a helper function. 

View on GitHub: https://github.com/w3c/web-platform-tests/pull/3324#issuecomment-235432909

Received on Tuesday, 26 July 2016 23:05:11 UTC