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

DS.Store type presence checks #4178

Merged
merged 1 commit into from
Mar 18, 2016

Conversation

courajs
Copy link
Contributor

@courajs courajs commented Feb 22, 2016

Fixes #4170.
Are you alright with generated tests like this?
I wasn't quite sure where to put these. serializerFor, createRecord, and a few others have their own test files, but many of these didn't really seem to have a home. It also seems a small shame to split these tests up when they're almost exactly the same.

@courajs courajs changed the title Store type presence checks DS.Store type presence checks Feb 22, 2016
@wecc
Copy link
Contributor

wecc commented Feb 26, 2016

Thanks @courajs!

We have a couple of other tests where we loop inside of one test(...) instead of generating multiple tests. This gives us the ability to set expected asserts (something like assert.expect(MODEL_NAME_METHODS .length) in this case).

See here for an example.

@courajs courajs force-pushed the store-type-presence-checks branch from 37cb617 to f6ea6da Compare March 5, 2016 13:47
@courajs
Copy link
Contributor Author

courajs commented Mar 5, 2016

Alrighty, I've updated to use only one test with many asserts

@@ -281,6 +282,7 @@ Store = Service.extend({
@return {DS.Model} record
*/
createRecord(modelName, inputProperties) {
assert("You need to pass a type to the store's createRecord method", isPresent(modelName));
Copy link
Member

Choose a reason for hiding this comment

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

Ember Data uses modelName for the type argument in most of these functions. I also know that type has also been used to refer to the same information. I think we should pick one term and be consistent. Right now I have a preference for modelName but I could easily be convinced to use type instead.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like it's not very consistent at the moment (see this and this), but I am in favor of modelName too, since it looks like the parameter for most method documentations is named modelName as well.

@courajs
Copy link
Contributor Author

courajs commented Mar 18, 2016

I've changed "type" to "model name" in the assertion messages

'serializerFor'
];

test("Calling Store methods with no type asserts", function(assert) {
Copy link
Member

Choose a reason for hiding this comment

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

Since #4223 the tests are also run in the production environment, where the assertions are stripped. That's why this test is failing.

You need to use testInDebug here instead of test:

import testInDebug from 'dummy/tests/helpers/test-in-debug';

Sorry you fell over this. This is my fault, I need to update CONTRIBUTING.md ASAP.

@pangratz
Copy link
Member

Can you squash into 1 commit and prefix with [BUGFIX beta]?

I think then this is good to go 🚀

bmac added a commit that referenced this pull request Mar 18, 2016
@bmac bmac merged commit f998732 into emberjs:master Mar 18, 2016
@bmac
Copy link
Member

bmac commented Mar 18, 2016

Thanks @courajs.

@courajs courajs deleted the store-type-presence-checks branch March 18, 2016 16:54
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.

None yet

4 participants