Skip to content
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.

Sign up for GitHub

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

feat(core): Print the name of the migration that cannot be reverted when using n8n db:revert #9473

Merged
merged 4 commits into from
May 23, 2024
+199 −71
Merged

feat(core): Print the name of the migration that cannot be reverted when using n8n db:revert #9473

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19065fa
feat(core): Print the name of the migration that cannot be reverted w…
despairblue May 21, 2024
9c88367
remove duplication from error message
despairblue May 21, 2024
94bf753
completely refactor the revert command and the tests
despairblue May 23, 2024
41a4701
remove commented out code
despairblue May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion packages/cli/src/commands/db/revert.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ export async function main(
}

if (!lastMigration.down) {
logger.error('The last migration was irreversible and cannot be reverted.');
const message = lastMigration.name
despairblue marked this conversation as resolved.
Show resolved Hide resolved
? `The last migration "${lastMigration.name}" was irreversible and cannot be reverted.`
: 'The last migration was irreversible and cannot be reverted.';
ivov marked this conversation as resolved.
Show resolved Hide resolved
logger.error(message);
return;
}

Expand Down
39 changes: 39 additions & 0 deletions packages/cli/test/unit/commands/db/revert.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,45 @@ test("don't revert the last migration if it had no down migration", async () =>
expect(dataSource.destroy).not.toHaveBeenCalled();
});

test('print migration name in error message is the migration has a name', async () => {
//
// ARRANGE
//
class TestMigration implements IrreversibleMigration {
name = 'Test Migration';

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.
ivov marked this conversation as resolved.
Show resolved Hide resolved
dataSource.migrations = migrations.map((M) => new M());

//
// ACT
//
await main(connectionOptions, logger, function () {
return dataSource;
} as never);
ivov marked this conversation as resolved.
Show resolved Hide resolved

//
// ASSERT
//
expect(logger.error).toHaveBeenCalledTimes(1);
ivov marked this conversation as resolved.
Show resolved Hide resolved
expect(logger.error).toHaveBeenCalledWith(
'The last migration "Test Migration" was irreversible and cannot be reverted.',
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as with error messages, should we avoid asserting on specific strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you ask me I'd say it depends.

If we just want to make sure that some error is thrown then we can apply DRY here similar to how it already is done with UM_FIX_INSTRUCTION.
On the other hand, matching against a literal makes sense if the error became part of the external contract and we want to have an extra assertion making sure it's not changed without acknowledging that the contract changed. E.g. if I change a constant and all tests pass I don't expect the FE to break. In that case asserting against a literal creates value because every time I change a test I have to assume that the FE could break.

In this case I can create a constant for the constant error message, for the dynamic one I will have to continue to construct it in the tests.

expect(dataSource.undoLastMigration).not.toHaveBeenCalled();
expect(dataSource.destroy).not.toHaveBeenCalled();
});

test('revert the last migration if it has a down migration', async () => {
//
// ARRANGE
Expand Down
Loading