- Notifications
You must be signed in to change notification settings - Fork923
chore: add database test fixture to insert non-unique linked_ids#12111
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Emyrk commentedFeb 12, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed by Graphite.Learn more about stacking. |
| @@ -0,0 +1,19 @@ | |||
| -- This is a deleted user that shares the same username and linked_id as the existing user below. | |||
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.
@coadler Is this ok to name this fixture different then the last migration?
Should I make this fixture to an earlier migration number?
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.
I wonder if you could also just add it in the base fixture 🤔 or wherever the linked_ids were introduced
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.
I was wondering that too. Was not sure what the protocol is on the immutability of test fixtures.
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.
I would probably add it as the same migration id thatuser_links was added, but I'm not super familiar with these. Maybe@mafredri can weight in
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.
I'll move it to an older one. I guess just migrations are immutable.
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, since the fixtures should never make their way into a customer deployment I think this should be fine.
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.
I moved it to000048_userdelete.up.sql. That is when deleting users was supported. Which is what this really is, adding a deleted user.
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 definitely, always feel free to go wild with the fixtures, and use them to place in wrong-use traps for the code. 😄
2883815 to78e90d6Compare78e90d6 to90e7164CompareAdded to the migration that added the delete field to users
| VALUES ('a0061a8e-7db7-4585-838c-3116a003dd21', 'githubuser@coder.com', 'githubuser', '\x', '2022-11-02 13:05:21.445455+02', '2022-11-02 13:05:21.445455+02', 'active', '{}', true) ON CONFLICT DO NOTHING; | ||
| INSERT INTO public.organization_members VALUES ('a0061a8e-7db7-4585-838c-3116a003dd21', 'bb640d07-ca8a-4869-b6bc-ae61ebb2fda1', '2022-11-02 13:05:21.447595+02', '2022-11-02 13:05:21.447595+02', '{}') ON CONFLICT DO NOTHING; | ||
| INSERT INTO public.user_links(user_id, login_type, linked_id, oauth_access_token) | ||
| VALUES('a0061a8e-7db7-4585-838c-3116a003dd21', 'github', '100', ''); |
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.
Will this trigger potential issues even with an empty token?
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.
Nah. If it does, that would be quite interesting.
It's in a fixture, so this never touches anything in a production environment.
Uh oh!
There was an error while loading.Please reload this page.
Related to#11861
I am working on a solution to the issue above. Adding test fixtures to ensure any added constraints handle these cases.
Before I even do that, I want to make sure our test fixtures accurately represent real database possibilities.