Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial migration of a subset of Google tests #3324

Closed
wants to merge 21 commits into from

Conversation

mwatson2
Copy link
Contributor

No description provided.

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/6721

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@wpt-pr-bot
Copy link
Collaborator

Reviewers for this pull request are: @ddorwin.

@jdsmith3000
Copy link
Contributor

The following comments are from Alexey in our Quality group:

  1. Code questions:
    1. Playback-temporary.js
      1. Was EventWatcher 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.
      2. 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?
      3. There are a few assert_any calls. According to testharness.js : “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.
      4. In onMessagefunction. 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?
    2. 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.
      2. 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(“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?
  2. 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.
  3. Unification of arguments indentation. For example in “playback-temporary.js” I can see “( event.target, _mediaKeySession );”, “( event.type, 'message');”, “(_mediaKeys);”, “( config.initDataType || event.initDataType,
  4. 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.

@mwatson2
Copy link
Contributor Author

This PR will be split into smaller ones, starting with #3355

@mwatson2
Copy link
Contributor Author

Regarding the comments above:
(1)(i)(a) Yes, we could consider EventWatcher, but I do not think it is urgent
(1)(i)(b) In #3355 I have removed the event checking. I will address this comment in a later PR with a version of the test that checks events
(1)(i)(c) ack
(1)(i)(d) Whether there is an individualization-request exchange depends on the key system and if there is we should just proxy it to the message handler. Following that we should expect the normal message exchange.
(1)(ii)(a) I will make the stepTest mandatory
(1)(ii)(b) Yes, using promise_test might be cleaner, but I'd prefer to leave that until after our immediate deadline
(2) Agreed - again I'd prefer to leave some style items until after our immediate deadline

@mwatson2
Copy link
Contributor Author

This PR has been split into smaller ones: #3355, #3359, #3360, #3361, #3362, #3363, #3364.

@mwatson2 mwatson2 closed this Jul 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants