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

Create Proper Migration tests #15116

Merged
merged 8 commits into from
Mar 24, 2021
Merged

Create Proper Migration tests #15116

merged 8 commits into from
Mar 24, 2021

Conversation

zeripath
Copy link
Contributor

Unfortunately our testing regime has so far meant that migrations do not
get proper testing.

This PR begins the process of creating migration tests for this.

Signed-off-by: Andrew Thornton art27@cantab.net

@zeripath zeripath added type/testing pr/wip This PR is not ready for review labels Mar 22, 2021
@zeripath
Copy link
Contributor Author

zeripath commented Mar 22, 2021

TODO:

  • wire into makefile
  • wire into .drone.yml

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 22, 2021
@zeripath zeripath changed the title WIP: Create Proper Migration tests Create Proper Migration tests Mar 22, 2021
@zeripath zeripath removed the pr/wip This PR is not ready for review label Mar 22, 2021
@zeripath
Copy link
Contributor Author

OK it's wired in..

The structure is a bit of a hack with the current inferior migration tests still being run from integrations/migration-test - these should probably be either moved in to migrations as proper tests or just removed once we have enough tests to be happy that they are no longer needed.

models/migrations/migrations_test.go Outdated Show resolved Hide resolved
models/migrations/migrations_test.go Show resolved Hide resolved
models/migrations/migrations_test.go Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor Author

Oh that's interesting there's a bug with drop table columns on mssql ...

@6543 6543 added this to the 1.15.0 milestone Mar 23, 2021
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 23, 2021
Unfortunately our testing regime has so far meant that migrations do not
get proper testing.

This PR begins the process of creating migration tests for this.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

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

Thanks for adding these! 💯

Do you think it would be good practice to add a comment block at the top of v176_test.go and v177_test.go to explain at high level what the migrations intend to do? Ex. "create index X", etc.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 24, 2021
@zeripath
Copy link
Contributor Author

I think such comments would probably be better on the migration functions themselves.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@6543 6543 merged commit 39ef6f8 into go-gitea:master Mar 24, 2021
@6543 6543 added the type/changelog Adds the changelog for a new Gitea version label Mar 24, 2021
@zeripath zeripath deleted the migration-tests branch March 24, 2021 19:36
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
@lafriks lafriks removed the type/changelog Adds the changelog for a new Gitea version label Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants