update task model tests #547
Conversation
|
I discussed this with @joshsmith and we think we should sort of look at ember-side model tests as if they were phoenix-side view tests. @filipecrosk did this when submitting his PR, by adding a test to assert all required attributes are accounted for. You can see an example at https://github.com/filipecrosk/code-corps-ember/blob/2197e7926fa6f3b1d30eee63a3705623e2a15f18/tests/unit/models/project-test.js#L17 Adding something like this here would probably bee a good idea. On one hand, we could look at this as "requiring the contributor to add a new attribute to the model twice", but really, we should look at a test like that as a fail-safe to make sure the contributor hasn't accidentally removed a model attribute. Sort of a "you did this, are you sure?". |
|
I've updated this to test attribute existence. I tried a few methods and the simplest was the |
| assert.ok(attributes.includes('markdown'), 'task should have the markdown attribute'); | ||
| assert.ok(attributes.includes('number'), 'task should have the number attribute'); | ||
| assert.ok(attributes.includes('taskType'), 'task should have the taskType attribute'); | ||
| assert.ok(attributes.includes('status'), 'task should have the status attribute'); |
There was a problem hiding this comment.
Would move up one to stay alpha-ordered.
| const relationship = get(task, 'relationshipsByName').get('user'); | ||
|
|
||
| assert.equal(relationship.key, 'user', 'has relationship with user'); | ||
| assert.equal(relationship.kind, 'belongsTo', 'kind of relationship is belongsTo'); |
There was a problem hiding this comment.
These tests are starting to seem really duplicative. Wondering if we can instead do something like assert.hasManyRelationship('task', 'user') or assert.belongsToRelationship('task', 'project') which just wraps everything.
There was a problem hiding this comment.
In fact, here we go: https://gist.github.com/he9qi/b6354a81a0672dc63294/
There was a problem hiding this comment.
@joshsmith ya that's a good idea. Want to put that in a separate PR? Then I can circle back and update all of these.
| @@ -17,6 +22,72 @@ test('it exists', function(assert) { | |||
| assert.ok(!!model); | |||
| }); | |||
|
|
|||
| test('it should have all of it\'s attributes', function(assert) { | |||
There was a problem hiding this comment.
The \' isn't needed here, since it's "its".
|
@WenInCode improved but could use some minor changes first, then you can decide whether to open a separate issue (we'll need one to circle back anyway) or handle here. |
|
@joshsmith I brought in those helpers, and addressed remaining feedback. How does this look now? |
| // source: https://gist.github.com/he9qi/b6354a81a0672dc63294 | ||
| export function testForHasMany(name, many) { | ||
| test('should have many ' + many, function(assert) { | ||
| const Model = this.store().modelFor(name); |
There was a problem hiding this comment.
I would insert assert.expect(2); before everything.
There was a problem hiding this comment.
sure I'll update this
| testForBelongsTo('task', 'user'); | ||
| testForHasMany('task', 'comments'); | ||
| testForHasMany('task', 'commentUserMentions'); | ||
| testForHasMany('task', 'taskUserMentions'); |
There was a problem hiding this comment.
This looks so much better, don't you think? Way easier to write, too.
There was a problem hiding this comment.
Ya it's a lot better
|
Rebase, squash, and merge! 🎉 |
WenInCode
force-pushed
the
update-task-model-tests
branch
from
1f8b7db
to
b44d038
Compare
What's in this PR?
Update
taskunit model testsReferences