-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix(migrate): added module name in migration #29
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
==========================================
+ Coverage 71.41% 71.49% +0.07%
==========================================
Files 37 37
Lines 1508 1512 +4
==========================================
+ Hits 1077 1081 +4
Misses 317 317
Partials 114 114
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Hey @cnlangzi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -314,7 +314,7 @@ func TestMigrate(t *testing.T) { | |||
PRIMARY KEY (id) | |||
);`), | |||
}, | |||
}) | |||
}, WithModule("tests")) |
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.
suggestion (testing): Consider adding a separate test case for WithModule
.
It's great to see the WithModule
option being used in the existing test. However, to ensure that the module
field is being set and used correctly throughout the migration process, consider adding a dedicated test case that specifically verifies the behavior and outcome of setting a module name. This could involve checking if the module
field is correctly populated in the sqle_migrations
table after a migration.
@@ -314,7 +314,7 @@ | |||
PRIMARY KEY (id) | |||
);`), | |||
}, | |||
}) | |||
}, WithModule("tests")) |
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.
suggestion (testing): Missing test for migration without module
.
Given the addition of the module
field as a mandatory column in the sqle_migrations
table, it would be beneficial to include a test case that attempts a migration without specifying a module through WithModule
. This can help verify the behavior or error handling when the module
field is not provided, ensuring robustness in scenarios where the module might be inadvertently omitted.
Here's the code health analysis summary for commits Analysis Summary
|
Fixes
module
in sqle_migrations table