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.

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

Bugfix/rollback all wrong order #3172

Merged

Conversation

Projects
None yet
2 participants
@leeallen337
Copy link
Contributor

commented Apr 28, 2019

This pull request should address, and fix, the bug reported in #3131

This bug looks like when trying to rollback all migrations it's running them in chronological order and, I think, attempting to rollback all migrations and not just the ones completed (which is also a problem). From this line in the Migrator, val[0] I think is a list of all migrations in chronological order rather than only attempting to rollback the completed migrations in reverse chronological order.

I updated the functionality, if all=true is passed in, to rollback all completed migrations in reverse chronological order

I updated the tests for the second migration to depend on the first. The reason I think these tests may have been passing previously is because each migration file was creating (on up) and dropping (on down) a distinct database table so the rollback order the migrations ran in didn't matter at all since no migration impacted something in another migration

It doesn't appear that the documentation found here needs any updates as the language seems correct

Happy to make any additional changes and/or updates

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Apr 28, 2019

@leeallen337 Tests shouldn't depend on each other at all. Would be better to introduce a third standalone one.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Apr 28, 2019

Would be great to also add test for the case when rollback affects only part of existing migrations (could be a unit test for Migrator)

@leeallen337

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@leeallen337 Tests shouldn't depend on each other at all. Would be better to introduce a third standalone one.

I completely agree that tests shouldn't depend on each other. Each one should be able to stand alone. Looking back at my description for the pull request I didn't explain myself well. I apologize for any misunderstanding. What I was trying to say was not that the existing tests depended on each other, but that none of the migrations used in the rollback tests themselves depended on each other, which is often the case in real applications (e.g. a later migration that adds a column to a table that was created in a previous migration) and that's why the tests were originally passing.

This test on master would fail if any migrations used for testing the rollbacks depend on other earlier migrations (which is what happens with the bug in the issue) but should now pass with the changes in this pull request along with the updates to the migrations. Note I didn't actually alter any tests.

However, instead of altering existing migrations, I'm up for adding in new ones and writing a new test if that's what you would prefer instead?

Would be great to also add test for the case when rollback affects only part of existing migrations (could be a unit test for Migrator)

This test may cover the scenario you mentioned but I could be completely off. Let me know what you think.

I appreciate your comments and I'm happy to help contribute 👍

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

@leeallen337 Thank you for the explanation! Yes, that makes sense, sorry for the misunderstanding.

By additional test I meant for this new piece of code:

               .filter((migration) => {
                  return completedMigrations.includes(migration.file);
                })

Wouldn't that test that you pointed at would be failing previously if it was covering this case as well?

@leeallen337

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Oh! I think I understand now. So you're asking for a test for something like the following situation:
A user has the following migrations in their migration folder:

Migrations Complete/Run? (Yes/No} Batch Number
20190323202737_migration-a.js Yes 1
2019042320311_migration-b.js Yes 2
2019052320251_migration-c.js No N/A

Adding a test to make sure that a rollback all=true only runs down for migration-b and then migration-a and NOT migration-c, correct?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

Yes, exactly! I believe previously that wasn't the case?

@leeallen337

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

I'm pretty sure you're correct that it wasn't previously the case. I'll add in some tests. Good catch!

@leeallen337

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

I added in a test for that filter functionality on running only the completed migrations in reverse chronological order. Let me know what you think or if you have any questions

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

Looks fantastic, thank you very much!

@kibertoad kibertoad merged commit a2ab754 into tgriesser:master Apr 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@leeallen337 leeallen337 deleted the leeallen337:bugfix/rollback-all-wrong-order branch Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.