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

Refactor MigrationAcceptanceTest to test for major version bumps #8154

Merged
merged 9 commits into from
Nov 30, 2021

Conversation

cgardens
Copy link
Contributor

What

  • The original test had a lot of duplicate logic, so I refactored it back some.
  • Modified the test so that it tests migrating from 0.17 to the current version. This includes testing that the user cannot directly upgrade past the faux major version bump (0.32.0) and tests that when following the procedure up updating 0.32.0 first, the user can upgrade to whatever version they want.
  • Added a few helpers for common patterns that I've seen along the way for handling waiting and handling properties.

@cgardens cgardens temporarily deployed to more-secrets November 20, 2021 02:16 Inactive
@cgardens cgardens temporarily deployed to more-secrets November 20, 2021 02:18 Inactive
@cgardens cgardens temporarily deployed to more-secrets November 20, 2021 02:23 Inactive
Comment on lines +23 to +25
.thenReturn(false)
.thenReturn(false)
.thenReturn(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't aware this was a thing, cool

Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

A few small comments but overall LGTM

logsToExpect.add("Version: 0.17.0-alpha-db-patch");
private void runAirbyte(final File dockerComposeFile,
final Properties env,
final VoidCallable assertionExecutable,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: calling this an assertionExecutable feels a little strange to me, because that makes me think that the function passed in should only be executing assertions. However in this PR you pass in the populateDataForFirstRun() function which also loads data into the db. Maybe something like postStartupExecutable would be better?

majorVersionBumpRun();
finalRun(targetVersion);
// run "faux" major version bump version
final File version32DockerComposeFile = MoreResources.readResourceAsFile("docker-compose.yaml");
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the version 0.32.0 docker-compose file also need to be preserved, similar to the docker-compose-migration-test-0-17-0-alpha.yaml file used above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call. i had added the file to source but wasn't referencing it here. will fix.

@cgardens cgardens temporarily deployed to more-secrets November 30, 2021 00:16 Inactive
@cgardens cgardens temporarily deployed to more-secrets November 30, 2021 00:45 Inactive
@cgardens cgardens force-pushed the cgardens/improve-auto-migration-test branch from 2bc7fc2 to 0f43bf6 Compare November 30, 2021 00:58
@cgardens cgardens temporarily deployed to more-secrets November 30, 2021 01:00 Inactive
@cgardens cgardens temporarily deployed to more-secrets November 30, 2021 01:57 Inactive
@cgardens cgardens merged commit ada2e17 into master Nov 30, 2021
@cgardens cgardens deleted the cgardens/improve-auto-migration-test branch November 30, 2021 04:14
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.

2 participants