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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for new RFC268 based testing API #190

Merged
merged 1 commit into from Feb 21, 2018

Conversation

Projects
None yet
2 participants
@simonihmig
Copy link
Contributor

simonihmig commented Feb 18, 2018

@rwjblue @Turbo87 here's a first attempt at implementing the new testing APIs for Mocha, as we discussed recently on Slack.

@rwjblue your gist was working almost perfectly! 馃憤
I added few tests, for application and rendering more or less based on the existing ones. For setupUnitTest some more specific cases. Tell me if you feel there is something missing!

Nevertheless there are some problems:

  • setupTest(), as it is called in ember-qunit, is already taken by ember-mocha for the old APIs. I named the function for now setupUnitTest. Would have been nice to having matching names, but seems we cannot do this in this case.
  • The apparent FIFO ordering of before/after hooks causes after hooks to be called with a cleared up context when called after setup*Test(), see here. I think that's what you were chatting about at the end of the recent conversation, right? 馃

@simonihmig simonihmig referenced this pull request Feb 19, 2018

Closed

Ember 3.0 compatibility #56

@@ -1,7 +1,6 @@
/* eslint-env node */
module.exports = {
browsers: [
'ie 9',

This comment has been minimized.

@Turbo87

Turbo87 Feb 19, 2018

Member

is this necessary?

This comment has been minimized.

@simonihmig

simonihmig Feb 19, 2018

Contributor

This is just to run our test suite with native async/await. Without that change, it would get transpiled, but then it failes because of the missing regenerator runtime. As we are testing in Chrome anyways, I though to just use the native implementation. FWIW, ember-qunit does the same

This comment has been minimized.

@Turbo87

Turbo87 Feb 19, 2018

Member

yeah, but changing the support matrix would need a major version bump and I'd like to avoid that for now.

we can't use async/await in this lib because then we would require that our users also support it natively or via regenerator. we can use it in the test suite (of the test suite 馃檲), but then we should use ember-maybe-import-regenerator-for-testing and the corresponding linting to make sure that we definitely do not use it in prod code.

This comment has been minimized.

@simonihmig

simonihmig Feb 19, 2018

Contributor

I added async/await only to the test suite, so users should not be affected by this! And this is the targets.js file of the dummy app, so only affecting our test builds, not that of users. So I think that should be fine?

I agree we have to make sure linting catches accidental use of async/await in the lib code itself, I will check this...

This comment has been minimized.

@Turbo87

Turbo87 Feb 19, 2018

Member

I wanted to write that removing it would prevent us from catching IE9 bugs in CI, but then I realized how much bullshit that was... 馃槅

seems fine, thanks

This comment has been minimized.

@simonihmig

simonihmig Feb 19, 2018

Contributor

Yes, unless you want to enter a world of pain, and use saucelabs, that would not really work! 馃槈
(heard browserstack would be better, but never tried it)


describe('setupUnitTest', function() {

describe('comtext setup', function() {

This comment has been minimized.

@Turbo87

Turbo87 Feb 19, 2018

Member

typo


setTimeout(resumeTest, 200); // resume test after timeout
// make sure pauseTest does not wait forever, if resumeTest fails
let timer = setTimeout(() => expect(false, 'resumeTest() did not work').to.be.true, 300);

This comment has been minimized.

@Turbo87

Turbo87 Feb 19, 2018

Member

did you check that this assertion works as expected if resumeTest() does indeed not work?

I think we should just throw new Error('resumeTest() did not work') instead of the always-failing assertion

This comment has been minimized.

@simonihmig

simonihmig Feb 19, 2018

Contributor

did you check that this assertion works as expected if resumeTest() does indeed not work?

Yes, kind of. I removed the previous line (setTimeout(resumeTest, 200);), so not calling resumeTest(), and the assertion catched that.

I think we should just throw new Error('resumeTest() did not work') instead of the always-failing assertion

Yeah, I tried that before, but somehow it was not working. Do you know, does chai just throw if the assertion fails, or is there some other magic going on? If not, I can try again...

This comment has been minimized.

@Turbo87

Turbo87 Feb 19, 2018

Member

chai just throws exceptions (AssertionError), nothing else as far as I'm aware. It's a relatively simple system compared to QUnit.

@@ -27,3 +40,59 @@ export {
it,
setResolver
};

export function setupUnitTest(options) {

This comment has been minimized.

@Turbo87

Turbo87 Feb 19, 2018

Member

can we move these implementations to dedicated files?

This comment has been minimized.

@simonihmig

simonihmig Feb 19, 2018

Contributor

Sure

import {
TestModule,
TestModuleForModel,
TestModuleForComponent,
TestModuleForAcceptance
} from 'ember-test-helpers';
import { assign } from '@ember/polyfills';

This comment has been minimized.

@Turbo87

Turbo87 Feb 19, 2018

Member

this is not available yet in Ember 2.0 and 2.4. I would prefer to make this a non-breaking change so if we can work around it by falling back to merge that would be great.

this is causing the CI failure btw :)

This comment has been minimized.

@simonihmig

simonihmig Feb 19, 2018

Contributor

Yes, I saw that. Will fix it.

@Turbo87 Turbo87 added the enhancement label Feb 19, 2018

@simonihmig simonihmig force-pushed the simonihmig:new-test-api branch 4 times, most recently from 2f6dfc9 to bc164f2 Feb 20, 2018

@Turbo87

This comment has been minimized.

Copy link
Member

Turbo87 commented Feb 20, 2018

setupTest(), as it is called in ember-qunit, is already taken by ember-mocha for the old APIs. I named the function for now setupUnitTest. Would have been nice to having matching names, but seems we cannot do this in this case.

setupTest() right now is always called with a string as the first argument, right? and the setupTest() function that we want to use from now on basically has no arguments since there are no hooks to pass into it, right?

so we could export a shared setupTest() function that delegates to the corresponding implementation based on the arguments that were passed in. 馃

@simonihmig

This comment has been minimized.

Copy link
Contributor

simonihmig commented Feb 20, 2018

@Turbo87 your comments should have been addressed now!

Ember 2.0/2.4 is still failing, but now unrelated to the Ember.assign issue (after fixing that, even more errors seem to be uncovered now). Seems the new testing APIs aren't supported in those versions anyway, as can be seen here. So we should probably skip the tests here as well? @rwjblue confirm?


afterEach(function() {
return teardownApplicationContext(this);
});

This comment has been minimized.

@Turbo87

Turbo87 Feb 20, 2018

Member

wouldn't this have to be written before the setupUnitTest() call due to the FIFO ordering?

This comment has been minimized.

@simonihmig

simonihmig Feb 20, 2018

Contributor

Hm, maybe yes, good catch! Right now it does not make a difference I think, because the implementation of teardownApplicationContext does not depend on it. But we should probably not rely on this...

I suppose we won't have a solution that "just works" for the general ordering problem? (other than to document it properly)

This comment has been minimized.

@Turbo87

Turbo87 Feb 20, 2018

Member

I suppose we won't have a solution that "just works" for the general ordering problem?

I do have an idea...

let hooks = setupTest();

hooks.beforeEach(function() { ... });
hooks.afterEach(function() { ... });

setupTest() maintains its own FIFO/LIFO queues and exposes them by returning an object with before/afterEach functions. The additional advantage is that the API becomes even closer to QUnit.

The disadvantage is that for Mocha users calling hooks.afterEach is unusual, but since this feature is only needed as an escape valve and probably shouldn't be needed by most endusers this might be fine 馃

This comment has been minimized.

@simonihmig

simonihmig Feb 20, 2018

Contributor

As the user has to be made aware of the problem anyway, and be told to use this (as you also mention) rather unusual way, I wonder if it is not equally appropriate and less "magic" to just tell them to move their afterEach calls before the setupTest() call?

This comment has been minimized.

@Turbo87

Turbo87 Feb 20, 2018

Member

move their afterEach calls before the setupTest() call?

that is not an option if you want to provide e.g. a setupPretender(hooks) helper that needs to setup both hooks

and it puts the hooks entirely out of order which isn't particularly nice to read :(

@Turbo87

This comment has been minimized.

Copy link
Member

Turbo87 commented Feb 20, 2018

So we should probably skip the tests here as well?

good point. yes, I guess in that case we can just skip those tests for now until we drop support for those Ember versions.

@simonihmig

This comment has been minimized.

Copy link
Contributor

simonihmig commented Feb 20, 2018

setupTest() right now is always called with a string as the first argument, right?

Seems you can skip the name, and just supply an options object: https://github.com/emberjs/ember-mocha/blob/master/addon-test-support/ember-mocha/setup-test-factory.js#L9-L12

And as setupUnitTest() also allows to pass an options object that it passes to setupContext() (see https://github.com/emberjs/ember-mocha/pull/190/files#diff-42b488ce2f091969dd98e2bbfa044642R13), this creates an ambiguity we cannot reliably resolve I think!

@simonihmig simonihmig force-pushed the simonihmig:new-test-api branch 2 times, most recently from 1ab1f6e to 9368345 Feb 20, 2018

@simonihmig

This comment has been minimized.

Copy link
Contributor

simonihmig commented Feb 20, 2018

@Turbo87 finally green! 馃榾

recent changes:

  • renamed setupUnitTest to setupContainerTest
  • skip new API tests for Ember <2.4
  • fixed tests for Ember 2.4
}

// copy over the original values
_assign(this, originalContext);

This comment has been minimized.

@Turbo87

Turbo87 Feb 20, 2018

Member

do we have to reset originalContext to {} here?

This comment has been minimized.

@simonihmig

simonihmig Feb 20, 2018

Contributor

I just fixed this in beforeEach, to prevent accidental leaks. Though I don't think it makes much of a difference in practice. I tried for quite some time to find a test case that would have been affected by this, but somehow didn't succeed ;)

@simonihmig simonihmig force-pushed the simonihmig:new-test-api branch from 9368345 to eae10ac Feb 20, 2018

@simonihmig simonihmig force-pushed the simonihmig:new-test-api branch from eae10ac to cf73491 Feb 20, 2018

@Turbo87 Turbo87 merged commit 74f83df into emberjs:master Feb 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Turbo87 Turbo87 changed the title [WIP] Add support for new RFC268 based testing API Add support for new RFC268 based testing API Feb 21, 2018

@simonihmig simonihmig deleted the simonihmig:new-test-api branch Feb 21, 2018

@simonihmig simonihmig referenced this pull request Aug 21, 2018

Merged

[ember-mocha] Update typings for ember-mocha 0.14 #28272

7 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment