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/serializer-test: Add RFC232 variants #5298

Merged
merged 4 commits into from
Dec 29, 2017

Conversation

alexander-alvarez
Copy link
Contributor

See #5292

Had questions on what we wanted the default to be so I implemented both
@Turbo87 @bmac

import { setupTest } from 'ember-qunit';
import { run } from '@ember/runloop';

module('serializer:<%= dasherizedModuleName %>', '<%= friendlyTestDescription %>', function(hooks) {
Copy link
Member

Choose a reason for hiding this comment

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

Should only be two arguments here (friendlyTestDescription and the function(hooks) {}...

@alexander-alvarez
Copy link
Contributor Author

@rwjblue fixed... sorry about that... thoughts on any of the two testing styles here?

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

I think having both tests by default is totally fine 🤔 ...

// Replace this with your real tests.
test('it exists', function(assert) {
let store = this.owner.lookup('service:store');
let serializer = this.owner.factoryFor('serializer:<%= dasherizedModuleName %>').create({ store });
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 should be .lookup (not factoryFor().create()) because serializers are singleton...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you need the store initialized on the serializer though, which lookup doesn't automatically do
is that something that should be addressed upstream?

Copy link
Member

Choose a reason for hiding this comment

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

Haha, this seems sorta bonkers. I submitted #5300 to simplify greatly 😺

Copy link
Member

Choose a reason for hiding this comment

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

@alexander-alvarez - Even without #5300, we should do store.serializerFor(<%= dasherizedModelName %>) here as that is the public API for getting a given things serializer. There are a few fallbacks that Ember Data handles nicely for you (e.g. use application if you don't have a model specific one) and we should go through the same common API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

understood 👍


let serializedRecord = record.serialize();

assert.ok(serializedRecord);
Copy link
Member

Choose a reason for hiding this comment

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

This should probably use some sort of assert.deepEqual

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@alexander-alvarez alexander-alvarez Dec 26, 2017

Choose a reason for hiding this comment

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

I was playing around with adding assert.deepEqual({}, serializedRecord); in my repo, and realized that if you've already implemented the model for which a serializer is being generated, at the moment we can't generate a test case that will pass when generated + when using the equality check.

I was under the impression that generated test cases should always pass when initially generated with a new model (e.g. running ember g serializer should generate a serializer and a passing test)-- do we want to continue to have this behavior?

@alexander-alvarez
Copy link
Contributor Author

559) [PhantomJS 2.1] integration/relationships/has_many - Has-Many Relationships: Accessing a hasMany backed by a link multiple times triggers only one request
     Same promise is returned both times

not sure the test failure is related @rwjblue

@rwjblue
Copy link
Member

rwjblue commented Dec 27, 2017

Restarted!

@alexander-alvarez
Copy link
Contributor Author

@rwjblue didn't realize -- it's failing on canary

Scenario ember-canary: FAIL
Command run: ember test

@bmac
Copy link
Member

bmac commented Dec 29, 2017

Looks like the canary issue has be resolved and this is now passing again. @rwjblue has your issue been addressed?

@rwjblue rwjblue merged commit 292702a into emberjs:master Dec 29, 2017
@alexander-alvarez alexander-alvarez deleted the serializer-test branch December 29, 2017 14:13
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