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

test: convert storage tests to typescript #217

Merged
merged 5 commits into from
May 5, 2020
Merged

test: convert storage tests to typescript #217

merged 5 commits into from
May 5, 2020

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented May 4, 2020

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:

  • mongodb tests aren't testing all that much, since they mock out the actual MongoDB part
  • same goes for sequelize. The tests are very complex and verbose. I didn't change anything about them other than converting to jest/typescript

Copy link
Member

@papb papb left a 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 😅

Comment on lines 48 to 57
expect(description).toMatchInlineSnapshot(`
Object {
"name": Object {
"allowNull": false,
"defaultValue": undefined,
"primaryKey": true,
"type": "VARCHAR(255)",
},
}
`);
Copy link
Member

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?

Copy link
Contributor Author

@mmkal mmkal May 5, 2020

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);
Copy link
Member

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?

Copy link
Contributor Author

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.

@papb
Copy link
Member

papb commented May 5, 2020

I think some of them could be dropped and replaced with integration tests, long term: [...]

Agreed

@mmkal mmkal merged commit e74f1fd into master May 5, 2020
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

2 participants