-
Notifications
You must be signed in to change notification settings - Fork 407
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
Complete tests for lib/record.js
#185
Conversation
test/delete.test.js
Outdated
.destroy('rec123') | ||
.then( | ||
function() { | ||
throw new Error('Promise unexpectly fufilled.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: "unexpectly" -> "unexpectedly" (here and below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/record.test.js
Outdated
expect(record.get('foo')).toBeUndefined(); | ||
expect(record.get('baz')).toEqual('qux'); | ||
|
||
expect(table._base.runAction).toHaveBeenCalledWith( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The invocation of _base.runAction
doesn't seem particularly important for the caller. A more robust test would involve verifying that an HTTP PUT request is issued. This file sets the precedent, so good on you for finding that and sticking to it! Improving the existing tests is in-scope for our work, but I'll be satisfied if you'd rather address this in a follow-up patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated these tests to use the same mocking we use in all other places.
test/delete.test.js
Outdated
function(err) { | ||
expect(err.statusCode).toBe(402); | ||
expect(err.message).toBe('foo bar'); | ||
done(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The done
callback can be omitted when tests return a Promise (here and below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Digging the pun
- Remove unneeded done - Fix spelling issue
And replace with the same mocking we use in all other files
@jugglinmike any other comments? |
None--this looks good to me! |
No description provided.