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

Clean up async tests using async/await #1337

Closed
joeyparrish opened this issue Mar 6, 2018 · 5 comments
Closed

Clean up async tests using async/await #1337

joeyparrish opened this issue Mar 6, 2018 · 5 comments
Assignees
Labels
status: archived Archived and locked; will not be updated type: code health A code health issue type: enhancement New feature or request
Milestone

Comments

@joeyparrish
Copy link
Member

joeyparrish commented Mar 6, 2018

Since Jasmine v2.7.0, Jasmine supports async functions and functions that return Promises. We should no longer have to do this:

    it('can be used before load()', function(done) {
      player.attach().then(() => {
        return player.load('test:sintel_compiled');
      }).catch(fail).then(done);
    });

Instead, we should be able to let the returned Promise manage both completion and failures:

    it('can be used before load()', function() {
      return player.attach().then(() => {
        return player.load('test:sintel_compiled');
      });
    });  // no catch(fail), no then(done), no "done" at all!

Or even use async functions which implicitly return Promises:

    it('can be used before load()', async function() {
      await player.attach();
      await player.load('test:sintel_compiled');
    });
@joeyparrish joeyparrish added the type: code health A code health issue label Mar 6, 2018
@joeyparrish joeyparrish added this to the Backlog milestone Mar 6, 2018
@vaage
Copy link
Contributor

vaage commented Mar 6, 2018

Just a small nit, according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function:

    it('can be used before load()', function() {
      await player.attach();
      await player.load('test:sintel_compiled');
    });

will need to be:

    it('can be used before load()', async function() {
      await player.attach();
      await player.load('test:sintel_compiled');
    });

@joeyparrish
Copy link
Member Author

Updated. Thanks for catching that!

shaka-bot pushed a commit that referenced this issue Mar 6, 2018
This adds attach/detach methods to replace the media element in the
Player constructor.  Now applications can take back control of the
media element or provide a reference later in the Player's life cycle.

This also allows applications to decide whether or not to set up
MediaSource in advance, through an optional argument on attach and
unload.  The default will be to set up MediaSource in advance, which
maintains current behavior.  This advanced setup of MediaSource can
improve load latency later.

This change also introduces async/await for the first time in the
project, which required changes to eslint config, Closure build
arguments, Babel & Babel-polyfill setup, and the esprima parser used
by our extern generator.

The use of async/await will improve readability in many places, and
these infrastructure changes to enable async/await should also unblock
issues #1336 and #1337.

Closes #1087

Change-Id: I0d6b4e0e2af27a6520a3d070fa92b7139b2cb8b0
@TheModMaker TheModMaker added the type: enhancement New feature or request label Mar 10, 2018
@joeyparrish joeyparrish modified the milestones: Backlog, v2.6 Mar 13, 2018
@theodab theodab self-assigned this Mar 13, 2018
@theodab
Copy link
Contributor

theodab commented Mar 22, 2018

I started on this, but it will have to be put on hold for a while due to issues with Closure compiler type inference and async functions.

shaka-bot pushed a commit that referenced this issue Jun 8, 2018
Issue #1337
Change-Id: I0cb350ad87d0b5448d9f87f7823dbf307131108b
@TheModMaker
Copy link
Contributor

TheModMaker commented Jun 11, 2018

Jasmine has expect(...).toThrow which allows us to assert that a function will throw an exception. We should look into adding one for async methods. There appears to be expectAsync, but I don't think it supports asserting the value of the rejection. For example:

// Instead of:
try {
  await foo();
  fail();
} catch (e) {
  expect(e).toEqual(expected);
}

// Instead we could add:
await expectAsync(foo()).toBeRejectedWith(expected);
// Or
await expectAsync(foo).toRejectWhenCalled(expected);

shaka-bot pushed a commit that referenced this issue Jun 12, 2018
Also fixed some formatting in media tests.

Issue #1337
Change-Id: Ifc003e3cb8f4b7a9920a0d2a121089eea4cf1bcd
shaka-bot pushed a commit that referenced this issue Jun 13, 2018
Unfortunately, due to problems with our version of Closure compiler,
not all of the tests in player_unit.js and player_integration.js can be
modified to use async/await.
A lot of them can, though, so this modifies those tests.

Issue #1337
Change-Id: I8e07283c1e51e5532ebcba5194d29466d950015d
shaka-bot pushed a commit that referenced this issue Aug 6, 2018
Issue #1337
Change-Id: Ib4d0bdb3e61b2ff54585941d1b0e20452612a3eb
shaka-bot pushed a commit that referenced this issue Oct 4, 2018
While working with DrmEngine I broke some tests. Why looking into what
part of the tests I broke I updated the tests to ES6.

Issue #1337
Change-Id: Ic865585c7ab0e8b858ef45fb02f1bcd0db6bcce2
@vaage vaage modified the milestone: v2.6 Nov 26, 2018
@TheModMaker
Copy link
Contributor

Handled as part of #1157.

@shaka-project shaka-project locked and limited conversation to collaborators Aug 9, 2019
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: code health A code health issue type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants