-
Notifications
You must be signed in to change notification settings - Fork 285
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
Switch postgres tests to run in a matrix of versions #2195
Conversation
d33ee54
to
2d9d84a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me; I added some comments.
No need to address this here, but after testing this branch locally, some tests are taking very long locally:
That also led me to discover that a bunch of tests are not running for testPostgresDatastoreWithoutCommitTimestamps
, in particular those in this if statement, is that intentional or an oversight, seems to me like an oversight. It feels like testPostgresDatastoreWithoutCommitTimestamps
has drifted from testPostgresDatastore
Now that tests run on their own machines, can we revert #2186?
This is intentional - these tests should not run if there is an "in progress" migration step |
2d9d84a
to
cde4bf8
Compare
I suspect the answer is "no": the main source of flakiness appears to be the resources of the machines getting exhausted and I suspect running the tests in parallel again will result in more flakiness; we'll have to try it to be sure though |
cde4bf8
to
67613e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
67613e5
to
7aea384
Compare
datastore: ["crdb", "mysql", "spanner"] | ||
steps: | ||
- uses: "actions/checkout@v4" | ||
if: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can put the if on the whole job?
No description provided.