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

ember-data should provide its blueprints #3813

Merged
merged 7 commits into from
Dec 18, 2015
Merged

ember-data should provide its blueprints #3813

merged 7 commits into from
Dec 18, 2015

Conversation

stefanpenner
Copy link
Member

  • add blueprints
  • test blueprints

@stefanpenner stefanpenner mentioned this pull request Oct 6, 2015
7 tasks
@knownasilya
Copy link
Contributor

Nice. Glad this is a thing.

@rwjblue
Copy link
Member

rwjblue commented Dec 14, 2015

@stefanpenner - Mind if I pick this up, rebase, and whatnot?

@stefanpenner
Copy link
Member Author

@rwjblue would appreciate that thanks:)

These shims are already provided by `lib/ember-data-shims.js`.
@rwjblue
Copy link
Member

rwjblue commented Dec 14, 2015

OK, rebased and passing. Do we want to block this on pulling in ember-cli-blueprint-test-helpers and adding tests here?

Also, if we do, does anyone object to using mocha for the node-land tests?

Sample test using ember-cli-blueprint-test-helpers + mocha: https://github.com/dockyard/ember-suave/blob/master/tests/blueprint-test.js

@rwjblue
Copy link
Member

rwjblue commented Dec 15, 2015

Added tests for blueprints.

@stefanpenner
Copy link
Member Author

Do we want to block this on pulling in ember-cli-blueprint-test-helpers and adding tests here?

Yes, I believe we need to get the tests integrated (or we will just troll everyone). ALthough I see some tests... I must be missing something.

Also, if we do, does anyone object to using mocha for the node-land tests?

no objections

@rwjblue
Copy link
Member

rwjblue commented Dec 15, 2015

@stefanpenner - Ya, I pulled them in already. Still tweaking to get them all passing/happy.

@rwjblue rwjblue force-pushed the addonified branch 2 times, most recently from cec896f to f63b3ca Compare December 15, 2015 02:39
@rwjblue rwjblue force-pushed the addonified branch 2 times, most recently from 5354378 to 1905212 Compare December 15, 2015 02:59
@rwjblue
Copy link
Member

rwjblue commented Dec 15, 2015

OK, I beleive this is ready for a review. I updated the blueprints from current ember-cli master, and ensured that tests were in place for each blueprint.

@stefanpenner
Copy link
Member Author

SGTM, we likely should get @trabus to take a look aswell

@stefanpenner
Copy link
Member Author

we may want to be sure the windows build passes, now that the node side is actually important...

@trabus
Copy link
Contributor

trabus commented Dec 15, 2015

I have all my extracted tests here in this branch: https://github.com/trabus/ember-cli-legacy-blueprints/blob/blueprints/

I've got the addon, dummy, and pod tests as well. It should be noted that I used blueprint-nodetest.js so it could be ignored with an .npmrc file. You can generate blueprint tests with the blueprint test helpers, and installing should generate the dotfile and a runner as well.

Here's the adapter tests for example: https://github.com/trabus/ember-cli-legacy-blueprints/blob/blueprints/blueprints/adapter/blueprint-nodetest.js

@trabus
Copy link
Contributor

trabus commented Dec 15, 2015

@rwjblue
Copy link
Member

rwjblue commented Dec 15, 2015

Thanks @trabus, I'll get this updated (stealing all of your hard work 👯).

@trabus
Copy link
Contributor

trabus commented Dec 15, 2015

(stealing all of your hard work 👯).

That was my intent from the get-go. :)

@trabus
Copy link
Contributor

trabus commented Dec 15, 2015

@rwjblue @stefanpenner do we want to go with using the blueprint name in the test and putting the them in node-tests/blueprints/?

@rwjblue
Copy link
Member

rwjblue commented Dec 15, 2015

@trabus - I'm playing with it the way you have it first (test co located with the blueprint itself).

@trabus
Copy link
Contributor

trabus commented Dec 15, 2015

@rwjblue I'm really okay either way. Honestly, the node-tests folder makes sense to me, and might be a good format to align with.

On the other hand, it is convenient to have the associated test with the blueprint, but then the naming becomes an issue that will likely be brought up.

@stefanpenner
Copy link
Member Author

node-tests folder makes sense to me, and might be a good format to align with.
Add more commits by pushing to the addonified branch on emberjs/data.

👍

@trabus
Copy link
Contributor

trabus commented Dec 15, 2015

I'll get the blueprint fixed then, and will move the blueprint-tests in legacy-blueprints to the node-tests folder. Tracking in this issue:
ember-cli/ember-cli-blueprint-test-helpers#4

@rwjblue
Copy link
Member

rwjblue commented Dec 16, 2015

Updated the infrastructure to use the base setup provided by [email protected] (using node-test/ folder as mentioned by @trabus above). Will update the actual tests tomorrow (I believe @trabus is updating them in the ember-cli-legacy-blueprints package so I'll just copy-pasta after that is done).

@bmac
Copy link
Member

bmac commented Dec 18, 2015

@rwjblue what is the status of this pr? I am planning on doing another 2.3 beta release today and wanted to see if this was ready to pull in.

@stefanpenner
Copy link
Member Author

I believe it is in working order, but @rwjblue should confirm as he has been helping push it to completion, their might be something I am not aware of pending.

@rwjblue
Copy link
Member

rwjblue commented Dec 18, 2015

Ya, this is good to land in next beta. I will continue working in additional PR's to flesh out the blueprint tests a bit more.

bmac added a commit that referenced this pull request Dec 18, 2015
ember-data should provide its blueprints
@bmac bmac merged commit 09ca817 into master Dec 18, 2015
@bmac
Copy link
Member

bmac commented Dec 18, 2015

Thanks for all the work @stefanpenner and @rwjblue.

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.

5 participants