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

Fix rollback on only one migration #1060

Merged
merged 2 commits into from
Apr 11, 2017
Merged

Conversation

moritz-h
Copy link
Contributor

@moritz-h moritz-h commented Mar 18, 2017

Fix #1059

I think error comes with this commit: bba44b3
If only only migration is left the first migration is selected as target instead of 0. So nothing is done.

I found a test case for this, but don't know why the test does not work:
https://github.com/cakephp/phinx/blob/v0.8.0/tests/Phinx/Migration/ManagerTest.php#L712

@rquadling
Copy link
Collaborator

A negative assertion, only tests what shouldn't happen, not what does happen. Hmmm.

@moritz-h
Copy link
Contributor Author

I played with the assertions. So ether add to assert not contains No migrations to rollback
or to contains some of this:

== 20120111235330 TestMigration: reverting
 == 20120111235330 TestMigration: reverted 0.0000s

@rquadling
Copy link
Collaborator

Hi.

If you can make the test assert positively (i.e. that when there is 1 or 0 migrations, the result is appropriate, then I can merge this.

@moritz-h
Copy link
Contributor Author

I added both now. Positive assertion for the "reverting" and "reverted" status output.
Also the negative assertion to not contain "no migrations", so for what ever could go wrong, the test could fail on this.

@rquadling rquadling merged commit f55f871 into cakephp:master Apr 11, 2017
@rquadling
Copy link
Collaborator

Thank you.

@glensc
Copy link
Contributor

glensc commented Jul 20, 2017

@moritz-h (since you are frequent contributor): do not link to branches, use git tag (or commit hash) to link to sourcecode, because the line (or even file) referenced gets out of sync, so the links in issue make no sense in the future.

@moritz-h
Copy link
Contributor Author

moritz-h commented Jul 20, 2017

@glensc ok, I try to do this in future.
edit: (done for the above link)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants