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

RFC: Drop "ember-mocha-adapter" in favor of a wrapper function #115

Closed
Turbo87 opened this issue Dec 7, 2016 · 6 comments
Closed

RFC: Drop "ember-mocha-adapter" in favor of a wrapper function #115

Turbo87 opened this issue Dec 7, 2016 · 6 comments

Comments

@Turbo87
Copy link
Member

Turbo87 commented Dec 7, 2016

We are currently using ember-mocha-adapter to implement async acceptance tests using the built-in async Ember helpers, without using done() or returning a Promise. The ember-mocha-adapter is implementing this by overriding the it() function and patching in our custom async handling. This is however error-prone and brittle as demonstrated by e.g. ember-cli/ember-cli-mocha#73 which shows that this approach will no longer work with Mocha 3.x.

A potential solution to this is using a wrapper function like this:

it('acceptance test works', emberAsync(function() {
  visit('/');
  andThen(function() {
    expect(currentURL()).to.equal('/foobar');
  });
}));

The emberAsync() function will call the passed in function and return a Promise waiting for any async behavior that was triggered inside the passed in function, e.g. by return wait() or return andThen(function() {}) if wait() is not yet available in Ember.

My initial spike used the function name async() instead of emberAsync(), but @rwjblue correctly pointed out that this will lead to naming conflicts with the upcoming async/await feature and we should try to avoid that. I'd be happy to hear other naming suggestions though.

see also teddyzeenny/ember-mocha-adapter#35 (comment)

/cc @rwjblue @trentmwillis @teddyzeenny @dgeb

@dwickern
Copy link

dwickern commented Dec 7, 2016

I use done() in my tests and I wasn't aware of this feature. I'd rather opt into this functionality as opposed to changing the behavior of mocha.

How is this better than async/await, which should work fine today (provided the necessary polyfills)?

it('acceptance test works', async function() {
  await visit('/');
  await andThen(function() {
    expect(currentURL()).to.equal('/foobar');
  });
});

@Turbo87
Copy link
Member Author

Turbo87 commented Dec 7, 2016

@dwickern it is not better than async/await, but async/await isn't that widely available and simply not the default recommendation yet.

In fact your example could probably even be simplified to this:

it('acceptance test works', async function() {
  await visit('/');
  expect(currentURL()).to.equal('/foobar');
});

in other words: andThen() will no longer be necessary if async helpers as used with await.

I'd rather opt into this functionality as opposed to changing the behavior of mocha.

that is precisely what I am proposing :)

@rwjblue
Copy link
Member

rwjblue commented Dec 7, 2016

This seems good to me, however it will require another round of blueprint updates based on version (which will be annoying) 😝

@Turbo87
Copy link
Member Author

Turbo87 commented Dec 7, 2016

@rwjblue I know, but this will only require an update to the acceptance-test blueprint, so it's not that bad :)

@Turbo87
Copy link
Member Author

Turbo87 commented Dec 7, 2016

maybe we should use the name usingAsyncHelpers(...) instead of emberAsync(...) to make the intent a bit clearer

@Turbo87
Copy link
Member Author

Turbo87 commented Dec 9, 2016

see #116 (comment)

@Turbo87 Turbo87 closed this as completed Dec 9, 2016
@Turbo87 Turbo87 added the wontfix label Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants