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

Trigger an assertion when calling findRecord with falsy (except 0) id #3795

Merged
merged 1 commit into from
Oct 2, 2015

Conversation

e00dan
Copy link
Contributor

@e00dan e00dan commented Sep 23, 2015

Attempt to close #2348. Do you think it is sufficient or should Ember.Error be thrown as @stefanpenner suggested here?

@@ -1,3 +1,5 @@
import { badIdFormatAssertion } from 'ember-data/system/store';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This causes an error:

 706 tests complete (9256 ms)
  706) integration/snapshot - DS.Snapshot: global failure
     Error: Could not find module ember-data/system/store

Is there any other way I can import exported module variables? I wonder why I can't use normal import like in HTMLBars. I also haven't found any example of using import in unit tests. I doubt they are real unit tests if they have access to all global namespaces, no imports. Is this because of old Ember CLI version?

PS: Tests on my local machine pass, appveyor build runs fine, only travis errors.

@wecc @bmac

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a limitation of our testing infrastructure. Modules can be access in dev mode but the production build (which I suspect travis is using) wraps Ember Data's module loader in a closure so tests (or user code) can't get at internal paths like 'ember-data/system/store'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll try to work around this. Do you plan to fix this in future?

@e00dan
Copy link
Contributor Author

e00dan commented Sep 24, 2015

Tests pass.

@bmac
Copy link
Member

bmac commented Sep 28, 2015

Looks good @kuzirashi. Do you mind squashing this pr into 1 commit?

@e00dan
Copy link
Contributor Author

e00dan commented Sep 29, 2015

Commits squashed. @bmac

@e00dan
Copy link
Contributor Author

e00dan commented Oct 1, 2015

Ping @bmac :S hope you notice and see if there's anything for this pr to get merged or you just didn't notice it. :P

bmac added a commit that referenced this pull request Oct 2, 2015
Trigger an assertion when calling `findRecord` with falsy (except 0) id
@bmac bmac merged commit 936d6dc into emberjs:master Oct 2, 2015
@bmac
Copy link
Member

bmac commented Oct 2, 2015

Thanks @kuzirashi.

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.

store.find('model', ''); shouldn't query 'api/models/'
2 participants