fix migrate:up for migrations disabling transactions #4550
Conversation
|
Bump |
|
@jeff-permiso Sorry for the delay. |
- also add tests for migrate:up, migrate:down when transactions are disabled
|
@kibertoad Sorry for the delay -- I've added tests around single up/down migrations without transactions, and refactored the method as requested. |
|
@kibertoad Any feedback on the tests or refactoring? Thanks much. |
| exports.up = async function () {}; | ||
|
|
||
| exports.down = async function (knex) { | ||
| if (isPostgreSQL(knex)) { |
There was a problem hiding this comment.
this doesn't seem to be the right place for skipping the test, could you move it into a test itself? there are plenty of examples of how db-specific tests are handled
There was a problem hiding this comment.
Moved to test
| ); | ||
| }); | ||
|
|
||
| it('should run', async () => { |
There was a problem hiding this comment.
does this test those migrations to simply not fail with an error? can we also assert that they actually did what we expect them to?
There was a problem hiding this comment.
The migrations don't actually do anything other than throw an error if called within a transaction. Added a comment to that effect.
| }); | ||
|
|
||
| it('should not error if all migrations have already been undone', function () { | ||
| return knex.migrate |
There was a problem hiding this comment.
can this be rewritten into async/await?
| ); | ||
| }); | ||
|
|
||
| it('should run', async () => { |
There was a problem hiding this comment.
The migrations don't actually do anything other than throw an error if called within a transaction. Added a comment to that effect.
| exports.up = async function () {}; | ||
|
|
||
| exports.down = async function (knex) { | ||
| if (isPostgreSQL(knex)) { |
There was a problem hiding this comment.
Moved to test
| }); | ||
|
|
||
| it('should not error if all migrations have already been undone', function () { | ||
| return knex.migrate |
|
What is PostgreSQL-specific for these tests, btw? Aren't transactions pretty universal? |
The method I was using to test being in a transaction wasn't. Rewrote to make more universal. |
|
Thanks a lot! |
does not respect configuration
due to treating the return value of getMigration as a value rather than a promise.
I could use advice on where to add tests for this behavior.