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

Run data_fixtures app migrations at the end of automated tests #1352

Open
shanbady opened this issue Aug 1, 2024 · 4 comments · Fixed by #1368
Open

Run data_fixtures app migrations at the end of automated tests #1352

shanbady opened this issue Aug 1, 2024 · 4 comments · Fixed by #1368
Labels
product:mit-open Issues related to the MIT Open product

Comments

@shanbady
Copy link
Contributor

shanbady commented Aug 1, 2024

Description/Context

We have a data_fixtures app that installs static fixtures - we currently dont run these in our automated test suit since a lot of our tests assume there is a clean slate when running since we don't explicitly setup()/teardown() - however this also means that if there is an issue with migrations in the data_fixtures app, it wont be revealed until we attempt a release.

Plan

  • run data migrations in our automated tests/CI after all tests have run so that we can catch errors in static fixture migrations
@sovsey sovsey added the product:mit-open Issues related to the MIT Open product label Aug 1, 2024
@jkachel
Copy link
Contributor

jkachel commented Aug 1, 2024

FWIW, this might be easy to do - the missing migrations script already will fail out if it gets an error back when it runs makemigrations so just updating it to run the data_fixtures migrations might be sufficient.

@jkachel
Copy link
Contributor

jkachel commented Aug 6, 2024

Should be resolved in #1368 (but typoed the ticket number in it so this didn't close automatically)

@jkachel jkachel closed this as completed Aug 6, 2024
@ChristopherChudzicki
Copy link
Contributor

@jkachel #1368 is very useful, but does it resolve this? We still don't run the data migrations on CI, we just check for conflicting migrations. The data migrations are manually written, so there could be an error in them.

Should we run RUN_DATA_MIGRATIONS=true ./manage.py migrate on CI after tests?

@jkachel
Copy link
Contributor

jkachel commented Aug 6, 2024

@jkachel #1368 is very useful, but does it resolve this? We still don't run the data migrations on CI, we just check for conflicting migrations. The data migrations are manually written, so there could be an error in them.

Should we run RUN_DATA_MIGRATIONS=true ./manage.py migrate on CI after tests?

The idea in #1368 was just to check for conflicts (since that was what prompted this) - but yes, it doesn't check for errors in the migrations themselves. (I think this would be caught anyway as someone has to approve the PR they're in but it wouldn't hurt to have an automated check too.) Reopening it for that.

Thinking about it a little more, though - the check for diverging paths is good in the context of the current PR/branch but I think the check should go a little further. The problem that prompted this was a PR that had a migration that merged after a PR that had a migration in data_fixtures merged. So, the CI check on either branch would have been fine. Maybe it should also rebase from main and check the migrations? Not sure if this is a good way to go about checking this (or if it needs to; if there do end up being conflicts they should now cause main to fail?).

@jkachel jkachel reopened this Aug 6, 2024
@jkachel jkachel removed their assignment Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product:mit-open Issues related to the MIT Open product
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants