Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

tests/arrays: Assert that result does not equal original. #56

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

Krinkle commented Jan 2, 2013

Without this assertion, the simple implementation (where there is no copy being made) will pass for both remove and removeWithoutCopy.

@rmurphey rmurphey commented on the diff Jan 3, 2013

tests/app/arrays.js
@@ -27,6 +27,9 @@ define([
expect(result).to.have.length(3);
expect(result.join(' ')).to.eql('1 3 4');
+
+ // make sure that you didn't change the original array instance
+ expect(result).not.to.equal(a);
@rmurphey

rmurphey Jan 3, 2013

Owner

A better test would be to ensure that the original array is unmodified, because that's what we really care about. Can you just do this in #58 and we can close this issue?

@Krinkle

Krinkle Jan 7, 2013

It is rather unlikely that an implementation will both make a copy and then modify both and return the copy. But ensuring the original is unmodified makes sense.

@Krinkle

Krinkle Jan 7, 2013

#58 and #59 are different pulls that include this due to merge conflicts, unrelated otherwise. Or do you want me to squash them into one pull request? (people usually prefer to keep them atomic and easy to review)

Krinkle commented Mar 6, 2013

See #59.

@Krinkle Krinkle closed this Mar 6, 2013

Contributor

beeeswax commented on 270313c May 31, 2013

I was just about to submit a pull request for this same change! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment