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

Rework migrations #1237

Closed
wants to merge 13 commits into from
Closed

Rework migrations #1237

wants to merge 13 commits into from

Conversation

ewjoachim
Copy link
Member

Closes #

WIP work, but I've been blocking for too long already on this.
This is a new take on acceptance tests:

  • They're run as subprocess, as before
  • But in a dedicated venv
  • And can be run with multiple configuration (but the same tests will run)

As a start, I created 4 tests:

  • Normal conditions (current code, all migrations)
  • Run on the last release of procrastinate with just the pre-migrations (currently failing! @medihack, see below)
  • Run on the current code with just the pre-migrations
  • Normal conditions but use aiopg (currently not working, due to not specifying that we need the aiopg extra in the venv)

What's missing:

  • Fix the 2 failing test
  • Add some other combinations ?
  • The operations we run on each test are very minimal, we should test a few more things (see existing acceptance tests)
  • Remove acceptance tests once we test (mostly?) everything in the new tests. and remove this ugly "2" in the folder name
  • Add LATEST_TAG envvar in the CI to avoid reading the latest tag from the git tags during the tests. I don't trust our CI git clone to do it properly. Instead use gh releases something
  • Add a Readme explaining what the hell this whole thing is and how to contribute

Successful PR Checklist:

  • Tests
    • (not applicable?)
  • Documentation
    • (not applicable?)

PR label(s):

The interesting error we got on running procrastinate 2.x with pre-migrations:

E           tests.acceptance2.utils.SubprocessException: Subprocess failed (code 1) with stdout:  and stderr: Launching a job: app.t1(a=1)
E           function procrastinate_defer_job_v1(unknown, unknown, smallint, unknown, unknown, jsonb, unknown) does not exist
E           LINE 1: SELECT procrastinate_defer_job_v1($1, $2, $3, $4, $5, $6, $7...
E                          ^
E           HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
E           Database error.

@ewjoachim ewjoachim requested a review from a team as a code owner November 11, 2024 22:30
@github-actions github-actions bot added the PR type: miscellaneous 👾 Contains misc changes label Nov 11, 2024
Co-authored-by: Joachim Jablon <ewjoachim@gmail.com>
Copy link

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  procrastinate
  manager.py
Project Total  

This report was generated by python-coverage-comment-action

@ewjoachim
Copy link
Member Author

Woo, it's green in the CI! Promising :) Thank you for the pair @medihack !

@medihack
Copy link
Member

For using nox, I started working in a new branch and also a new PR #1245

@ewjoachim ewjoachim closed this Dec 1, 2024
@ewjoachim ewjoachim deleted the new-acceptance-tests branch December 1, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR type: miscellaneous 👾 Contains misc changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants