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

New testing API #84

Merged
merged 13 commits into from Sep 8, 2016
Merged

New testing API #84

merged 13 commits into from Sep 8, 2016

Conversation

@Turbo87
Copy link
Member

Turbo87 commented Aug 12, 2016

The old testing API is sort-of using inheritance on the describe() function to setup and teardown all the necessary pieces. The proposed new testing API is using composition instead via the new setupModuleTest() functions.

  • Replace custom it() with original from Mocha
  • Extract new setupModuleTest() and related functions
  • Add tests for the new functions
  • Update README to the new API
  • Add deprecation messages to the old API

Before:

import { expect } from 'chai';
import { describeModule, it } from 'ember-mocha';

describeModule(
  'controller:users',
  'Unit | Controller | users',
  {
    // Specify the other units that are required for this test.
    // needs: ['controller:foo']
  },
  function() {
    // Replace this with your real tests.
    it('exists', function() {
      let controller = this.subject();
      expect(controller).to.be.ok;
    });
  }
);

After:

import { expect } from 'chai';
import { it, describe } from 'mocha';
import { setupModuleTest } from 'ember-mocha';

describe('Unit | Controller | users', function() {
  setupTest('controller:users', {
    // Specify the other units that are required for this test.
    // needs: ['controller:foo']
  });

  // Replace this with your real tests.
  it('exists', function() {
    let controller = this.subject();
    expect(controller).to.be.ok;
  });
});

/cc @rwjblue @stefanpenner

@Turbo87 Turbo87 force-pushed the Turbo87:new-api branch from a4878aa to 087f4b7 Aug 12, 2016
@rwjblue
Copy link
Member

rwjblue commented Aug 12, 2016

I like it! Would like @dgeb and/or @stefanpenner to review also.

Definitely need deprecations on the existing things I think...

@dgeb
Copy link
Member

dgeb commented Aug 12, 2016

I like this approach too. I'm in favor of using mocha's describe and it instead of overriding.

My only real issue is with the method name setupModuleTest. Can we drop Module and just go with setupTest? Then there would be setupTest, setupModelTest, setupComponentTest, and setupAcceptanceTest.

I regret cargo-culting the term Module over from QUnit (and ember-qunit) and this seems a good chance to drop it.

Open to suggestions here.

@Turbo87
Copy link
Member Author

Turbo87 commented Aug 12, 2016

@dgeb I'd be fine with setupTest(), although in the future we might switch to setupUnit/Integration/AcceptanceTest() anyway once the RFC of @rwjblue gets implemented.

@dgeb
Copy link
Member

dgeb commented Aug 12, 2016

Yes, good to consider the implications of the testing RFC and aim for consistency with ember-qunit.

@Turbo87
Copy link
Member Author

Turbo87 commented Aug 12, 2016

pinging mr. @cowboyd! @rwjblue said you might have an opinion on this too and he'd like to hear it... 😉

@Turbo87
Copy link
Member Author

Turbo87 commented Aug 12, 2016

@dgeb renamed it to setupTest()

@rwjblue added the deprecation message

should be good to go now unless you have any more suggestions

@cowboyd
Copy link

cowboyd commented Aug 12, 2016

@Turbo87 This looks like a much welcome improvement!

@Turbo87 Turbo87 force-pushed the Turbo87:new-api branch from 30143c3 to 31327bc Aug 15, 2016
@Turbo87
Copy link
Member Author

Turbo87 commented Aug 21, 2016

@rwjblue can we merge this? any more comments?

@Turbo87
Copy link
Member Author

Turbo87 commented Aug 31, 2016

1️⃣ 2️⃣ 3️⃣ :shipit:

@rwjblue
Copy link
Member

rwjblue commented Aug 31, 2016

I'm 👍 . I'd like a 👍 /👎 from @dgeb and/or @trentmwillis before landing...

@Turbo87
Copy link
Member Author

Turbo87 commented Aug 31, 2016

Given that I addressed everything in #84 (comment) I assumed that he was 👍 too

@Turbo87
Copy link
Member Author

Turbo87 commented Sep 7, 2016

@dgeb @trentmwillis ping 😉

@trentmwillis
Copy link
Member

trentmwillis commented Sep 8, 2016

👍 didn't review the code, but I agree with this approach. Primarily, I like that it provides better separation between the default test framework and the Ember-specific test helpers. I also believe this will make it easier to retain all the default behaviors of the underlying framework (something we've had issues with in ember-qunit).

@dgeb
Copy link
Member

dgeb commented Sep 8, 2016

Given that I addressed everything in #84 (comment) I assumed that he was 👍 too

Yep, thanks for addressing my concerns @Turbo87. Let's ship this! :shipit:

@dgeb dgeb merged commit 5b102bd into emberjs:master Sep 8, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Turbo87
Copy link
Member Author

Turbo87 commented Sep 8, 2016

🎉

@Turbo87 Turbo87 deleted the Turbo87:new-api branch Sep 8, 2016
@Turbo87
Copy link
Member Author

Turbo87 commented Sep 9, 2016

@dgeb @rwjblue any plans for a 0.9 release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.