Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Included tests by adding .test extension #1448

Merged

Conversation

excitement-engineer
Copy link
Contributor

@excitement-engineer excitement-engineer commented Dec 23, 2017

Included Query.test.js and test-utils.test.js into the test suite by adding the .test extension.

@excitement-engineer excitement-engineer changed the title Updated the name of Query test Included tests by adding .test extension Dec 23, 2017
@rosskevin
Copy link
Contributor

looks good but tests aren't running again due to what looks like a conflict again with prettier/tslint. I've never used that package you added so not sure if it actually solves it here.

@apollo-cla
Copy link

Warnings
⚠️

❗ Big PR

Generated by 🚫 dangerJS

@excitement-engineer
Copy link
Contributor Author

cc @rosskevin, I finally fixed the tests:)

Copy link
Contributor

@rosskevin rosskevin left a comment

Choose a reason for hiding this comment

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

I'm a little tentative on just matching snapshots because they can be easily updated/committed, but all the same it is a reliable assertion method - we probably need to lockdown PR merge settings so that at least one review is required before merging to ensure no bad snapshot updates.

@rosskevin rosskevin merged commit ada779b into apollographql:master Dec 24, 2017
@excitement-engineer
Copy link
Contributor Author

I agree with you. I was hesitant to update to snapshots as well but I found no other clean way to run the assertions. Note, other test files use a toEqualJson() assertion (which is implemented in this lib), however this is broken causing errors as soon as the assertion fails. Something we should correct in the future.

@rosskevin
Copy link
Contributor

I had to add toEqualJson for the jest upgrade because the comparison of our hardcoded data did not have a Symbol(id) in it, and the json comparison is what we desired.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants