-
Notifications
You must be signed in to change notification settings - Fork 463
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
Add more substantial tests for compute migrations #9811
Conversation
One thing worth calling out: is the format of the migration plpsql tests what we want? Would it make more sense to do something like "if neon_superuser is granted access to func_xyz, write a SQL query for func_xyz?" |
7242 tests run: 6890 passed, 0 failed, 352 skipped (full report)Flaky tests (6)Postgres 17
Postgres 14
Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
b34260a at 2024-12-23T17:26:30.663Z :recycle: |
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 don’t think control plane has anything to say here, so LGTM.
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.
Left some suggestions, but otherwise looks great!
6fa910d
to
2d5d92c
Compare
I think this is pretty good. Perhaps we could add one more test which tests the following scenario: BEGIN;
-- Run migration
-- Fail, causing transaction ABORT
-- Update migration ID
COMMIT; and maybe BEGIN;
-- Run migration
-- Update migration ID
-- Fail, causing transaction ABORT
COMMIT; The purpose would be to make sure that the two operations (running the migration and updating the persisted migration ID) are transactional. Let me know if that sounds worthwhile, and I will add it. |
2d5d92c
to
dfe6cc3
Compare
Of course when you're reading directory entries they aren't sorted 😆 |
dfe6cc3
to
183df72
Compare
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 really good to me. See my final comment and I think it's ready for merge
The previous tests really didn't do much. This set should be quite a bit more encompassing. Signed-off-by: Tristan Partin <[email protected]>
183df72
to
b34260a
Compare
The previous tests really didn't do much. This set should be quite a bit more encompassing.