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

DBW: update smokehouse tests to do deep equals #1457

Merged
merged 2 commits into from
Jan 12, 2017

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Jan 12, 2017

R: @brendankenny all

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM. Excited to have this checking in dbw_tester after all the work you've put in to test all these terrible corner cases :)

@paulirish @patrickhulce for a second opinion on this using a length property solution we came up with for checking array lengths. There is precedence (some things that take an array will actually take anything with a length property, e.g. the first parameter of Array.from()), but one weird thing is that we only check expected's own enumerable properties (via Object.keys), but currently allow the matching property on actual to be anywhere on the prototype chain (which is why checking length works).

Is that unexpected/confusing? The only other option seems to be finish adding function comparators to smokehouse and do a (actual) => actual.length === 10 for the expected values.

@@ -345,6 +346,7 @@
}
</script>

<!-- PASS: not in header, does not block rendering -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh yeah actually could you comment that it's there to polyfill promise and make sure we don't break :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@patrickhulce
Copy link
Collaborator

not a huge fan of the nested length object compared to assert/expect/should, feels a little hackier to me, but I'm fine for now until we actually support that.

@brendankenny
Copy link
Member

🚬
🔦🔍

@brendankenny brendankenny merged commit ce79273 into master Jan 12, 2017
@ebidel
Copy link
Contributor Author

ebidel commented Jan 12, 2017

Trav, you're my boyyyyyyy.

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.

None yet

3 participants