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

blueprints/mixin-test: Added RFC-232 variant #15950

Merged
merged 1 commit into from
Dec 9, 2017

Conversation

boyanyordanov
Copy link
Contributor

Progress for #15933

A couple of things I am not sure about:

  • I wasn't sure whether the setupTest call is needed for mixing test (no setup was required before) and the codemod from https://github.com/rwjblue/ember-qunit-codemod didn't add it. I added it anyway, it looks logical after reading the RFC a couple of times.

  • and also there's an addon version of this blueprint. Should there also be an updated version for rfc232 for that as well ?

P.S. Sorry If the questions are stupid. I am still learning how all this works under the hood.

import { setupTest } from 'ember-qunit';

module('Unit | Mixin | foo', function(hooks) {
setupTest(hooks);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we necessarily need the setupTest() call here since we're not using the container for anything 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove it.

Copy link
Contributor Author

@boyanyordanov boyanyordanov Dec 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more question. Should I squash the commits after that, since it's a small change reverting some of this ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I think that would be good, thanks! :)

@Turbo87
Copy link
Member

Turbo87 commented Dec 8, 2017

also there's an addon version of this blueprint. Should there also be an updated version for rfc232 for that as well ?

I guess you are talking about the addon.js fixture file, right? I don't think we necessarily need an rfc232 version of that since the only thing that changes is the import path. but feel free to add it if you think that it might be useful :)

@boyanyordanov
Copy link
Contributor Author

I got confused a bit. Yeah, I was talking about the fixture. Looking at it now it doesn't seem that necessary to add another test just for the path replacement there.

Thanks a lot for the help!

@boyanyordanov
Copy link
Contributor Author

Done. Please let me know if additional changes are needed.

@rwjblue rwjblue merged commit 5b61b7e into emberjs:master Dec 9, 2017
@rwjblue
Copy link
Member

rwjblue commented Dec 9, 2017

Awesome, thank you!

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

Successfully merging this pull request may close these issues.

3 participants