-
-
Notifications
You must be signed in to change notification settings - Fork 154
fix: alter column uppercase type #154
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
Conversation
3. Run `npm test` while you work | ||
To start developing, run `npm run dev`. It will set up the database with Docker for you. The server will restart on file change. | ||
|
||
If you are fixing a bug, you should create a new test case. To test your changes, add the `-u/--updateSnapshot` flag to `jest` on the `test:run` script, run `npm run test`, and then review the git diff of the snapshots. Depending on your change, you may see `id` fields being changed - this is expected and you are free to commit it, as long as it passes the CI. Don't forget to remove the `-u/--updateSnapshot` flag when committing. |
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.
@soedirgo we should use Property matchers if the id
is changing and we don't care. We shouldn't recommend doing updateSnapshot
because it will too easily introduce regression
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.
Yeah I've started using it on newer tests - I deferred using it when I did the Jest migration because it was a pain to apply for all cases (some id
are nested in objects in arrays - need preprocessing there). I'll create an issue to use property matchers on existing tests.
For now I think it's ok to temporarily keep id
s and use --updateSnapshot
.
expect(res).toMatchInlineSnapshot( | ||
{ | ||
data: { | ||
id: expect.stringMatching(/^\d+\.\d+$/), |
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 already have a property matcher? any reason for the comment above?
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.
Answered above #154 (comment)
🎉 This PR is included in version 0.27.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fix: alter column uppercase type
Fixes supabase/supabase#3553.