-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
test: convert storage tests to typescript #217
Conversation
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.
Excellent PR! It's been a while since I enjoy reviewing a PR this much. My knowledge of Jest is greater than zero now. 🚀
I have just one minor whitespace fix request 😅
test/storages/sequelize.test.ts
Outdated
expect(description).toMatchInlineSnapshot(` | ||
Object { | ||
"name": Object { | ||
"allowNull": false, | ||
"defaultValue": undefined, | ||
"primaryKey": true, | ||
"type": "VARCHAR(255)", | ||
}, | ||
} | ||
`); |
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.
Looks like you forgot to fix this last line in your use tabs
commit (b840f0e)
Also, by the way, I'm curious - does Jest automatically outdents the inline snapshot?
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.
Good catch - and yes jest is supposed to handle this automatically, but I think I need to tweak some of the lint config so it can tell that we like tabs over spaces. I'll do that in another PR and fix this one manually.
}); | ||
|
||
return storage.executed().then(migrations => { | ||
expect(Object.keys(migrations)).toHaveLength(0); |
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.
Just out of curiosity - wouldn't expect(migrations).toEqual({})
work?
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.
Yes it probably would - this file is a straight up port, I just ran jest-codemods and cleaned up some of the context variables, I didn't touch any of the assertions.
Agreed |
This moves the storage tests out of legacy-tests and into typescript/jest-land.
There are some changes I want to make to a few of the storage helpers, and how
Umzug
is passed them, so I wanted to get them tested with the new system first. I think some of them could be dropped and replaced with integration tests, long term: