Skip to content

Conversation

stevemao
Copy link
Contributor

No description provided.

@@ -100,6 +100,13 @@ it('should preserve property order', function () {
assert.equal(Object.keys(target).join(''), letters);
});

it('target can be primatives', function () {

Choose a reason for hiding this comment

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

Although I'd rather see AVA as testing framework, if mocha is used, the convention of titles is more like this:

it('should accept primitives as target', function () {

@@ -5,7 +5,7 @@ var assert = require('assert');
Object.assign = undefined;
var objectAssign = require('./');

it('should have the correct length', function () {
it('should expect at least 2 arguments', function () {
Copy link
Owner

Choose a reason for hiding this comment

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

This is incorrect. It's testing how many arguments are there in the code, just what's passed.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, don't do unrelated changes when doing a PR ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... from mdn, it should mean the number of arguments expected by the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

arguments.length is how many arguments.

@sindresorhus sindresorhus changed the title add missing test for target being a primative add missing test for target being a primitive Jul 15, 2016
@stevemao
Copy link
Contributor Author

I removed unrelated changes

@sindresorhus sindresorhus merged commit 066439e into sindresorhus:master Jul 15, 2016
@sindresorhus
Copy link
Owner

Thanks :)

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