-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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(core): Don't revert irreversibble migrations #9105
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm only wrapping @krynble I opted not to throw in the wrapper here, because to test that I would need to re-implement |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,134 @@ | ||
import { main } from '@/commands/db/revert'; | ||
import { mockInstance } from '../../../shared/mocking'; | ||
import { Logger } from '@/Logger'; | ||
import * as DbConfig from '@db/config'; | ||
import type { IrreversibleMigration, ReversibleMigration } from '@/databases/types'; | ||
import type { DataSource } from '@n8n/typeorm'; | ||
import { mock } from 'jest-mock-extended'; | ||
|
||
const logger = mockInstance(Logger); | ||
|
||
afterEach(() => { | ||
jest.resetAllMocks(); | ||
}); | ||
|
||
test("don't revert migrations if there is no migration", async () => { | ||
// | ||
// ARRANGE | ||
// | ||
const connectionOptions = DbConfig.getConnectionOptions(); | ||
// @ts-expect-error property is readonly | ||
connectionOptions.migrations = []; | ||
const dataSource = mock<DataSource>({ migrations: [] }); | ||
|
||
// | ||
// ACT | ||
// | ||
await main(connectionOptions, logger, function () { | ||
return dataSource; | ||
} as never); | ||
|
||
// | ||
// ASSERT | ||
// | ||
expect(logger.error).toHaveBeenCalledTimes(1); | ||
expect(logger.error).toHaveBeenCalledWith('There is no migration to reverse.'); | ||
expect(dataSource.undoLastMigration).not.toHaveBeenCalled(); | ||
expect(dataSource.destroy).not.toHaveBeenCalled(); | ||
}); | ||
|
||
test("don't revert the last migration if it had no down migration", async () => { | ||
// | ||
// ARRANGE | ||
// | ||
class TestMigration implements IrreversibleMigration { | ||
async up() {} | ||
} | ||
|
||
const connectionOptions = DbConfig.getConnectionOptions(); | ||
const migrations = [TestMigration]; | ||
// @ts-expect-error property is readonly | ||
connectionOptions.migrations = migrations; | ||
const dataSource = mock<DataSource>(); | ||
// @ts-expect-error property is readonly, and I can't pass them the `mock` | ||
// because `mock` will mock the down method and thus defeat the purpose | ||
// of this test, because the tested code will assume that the migration has a | ||
// down method. | ||
dataSource.migrations = migrations.map((M) => new M()); | ||
|
||
// | ||
// ACT | ||
// | ||
await main(connectionOptions, logger, function () { | ||
return dataSource; | ||
} as never); | ||
|
||
// | ||
// ASSERT | ||
// | ||
expect(logger.error).toHaveBeenCalledTimes(1); | ||
expect(logger.error).toHaveBeenCalledWith( | ||
'The last migration was irreversible and cannot be reverted.', | ||
); | ||
expect(dataSource.undoLastMigration).not.toHaveBeenCalled(); | ||
expect(dataSource.destroy).not.toHaveBeenCalled(); | ||
}); | ||
|
||
test('revert the last migration if it has a down migration', async () => { | ||
// | ||
// ARRANGE | ||
// | ||
class TestMigration implements ReversibleMigration { | ||
async up() {} | ||
|
||
async down() {} | ||
} | ||
|
||
const connectionOptions = DbConfig.getConnectionOptions(); | ||
// @ts-expect-error property is readonly | ||
connectionOptions.migrations = [TestMigration]; | ||
const dataSource = mock<DataSource>({ migrations: [new TestMigration()] }); | ||
|
||
// | ||
// ACT | ||
// | ||
await main(connectionOptions, logger, function () { | ||
return dataSource; | ||
} as never); | ||
|
||
// | ||
// ASSERT | ||
// | ||
expect(logger.error).not.toHaveBeenCalled(); | ||
expect(dataSource.undoLastMigration).toHaveBeenCalled(); | ||
expect(dataSource.destroy).toHaveBeenCalled(); | ||
}); | ||
|
||
test('throw if a migration is invalid, e.g. has no `up` method', async () => { | ||
// | ||
// ARRANGE | ||
// | ||
class TestMigration {} | ||
|
||
const connectionOptions = DbConfig.getConnectionOptions(); | ||
// @ts-expect-error property is readonly | ||
connectionOptions.migrations = [TestMigration]; | ||
const dataSource = mock<DataSource>({ migrations: [new TestMigration()] }); | ||
|
||
// | ||
// ACT | ||
// | ||
await expect( | ||
main(connectionOptions, logger, function () { | ||
return dataSource; | ||
} as never), | ||
).rejects.toThrowError( | ||
'At least on migration is missing the method `up`. Make sure all migrations are valid.', | ||
); | ||
|
||
// | ||
// ASSERT | ||
// | ||
expect(dataSource.undoLastMigration).not.toHaveBeenCalled(); | ||
expect(dataSource.destroy).not.toHaveBeenCalled(); | ||
}); |
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.
If you have a better idea for unit testing, let me know. But everything I tried was messy due to
jest.mock
not working nicely with typeorm, because it will mock the decorators which then stop working andjest-extended-mock
will mock based on the type that is used to to retrieve the object, not the actual implementation, and thus will actually add methods which I rely on not existing, likeMigration.down
.Using DI meant changing
Db.init
to take custom data source options and eventually led to the test being in an infinite loop that I didn't investigate for long.So this is where I ended up for now.