MPT-18132: add command to run a single migration#44
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds positional Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
svazquezco
force-pushed
the
MPT-18132-add-command-to-run-a-single-migration
branch
from
2562d8f to
c26a494
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/cli/test_cli_local_storage.py (2)
54-73: Parameterize this with the existing full-data migration test to avoid duplicate assertions.This test mirrors the assertions in the prior data migration test with only CLI args changing. Consider combining them into a single parametrized test.
Suggested refactor
-@freeze_time("2025-04-06 13:10:30") -@pytest.mark.usefixtures("data_migration_file", "schema_migration_file") -def test_migrate_data_migration(migration_state_file, runner, log): - result = runner.invoke(app, ["migrate", "--data"]) +@freeze_time("2025-04-06 13:10:30") +@pytest.mark.usefixtures("data_migration_file", "schema_migration_file") +@pytest.mark.parametrize( + "cli_args", + [ + ["migrate", "--data"], + ["migrate", "--data", "fake_data_file_name"], + ], +) +def test_migrate_data_migration(cli_args, migration_state_file, runner, log): + result = runner.invoke(app, cli_args) # asserts unchanged - -@freeze_time("2025-04-06 13:10:30") -@pytest.mark.usefixtures("data_migration_file", "schema_migration_file") -def test_migrate_data_single_migration(migration_state_file, runner, log): - result = runner.invoke(app, ["migrate", "--data", "fake_data_file_name"]) - # asserts unchangedAs per coding guidelines: "Use
@pytest.mark.parametrizewhen testing permutations of the same behavior" and "Avoid duplicating test logic; extract shared setup into fixtures".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli/test_cli_local_storage.py` around lines 54 - 73, Combine the duplicate assertions by parameterizing the tests: replace test_migrate_data_single_migration and the existing full-data migration test with a single parametrized test (e.g., test_migrate_data_variants) that takes the CLI args (like ["migrate","--data","fake_data_file_name"] and the full-data args) and the expected migration_id/order_id/started_at/applied_at; use `@pytest.mark.parametrize` to pass both argument sets and reuse the same fixtures (migration_state_file, runner, log) and assertions currently inside test_migrate_data_single_migration (including checking result.exit_code, migration_state_file JSON contents, and log/output strings) so you avoid duplicate assertions and keep shared setup in the fixtures.
124-141: Parametrize these two error-path tests to reduce duplication.Both tests run the same command shape and differ only by migration_id and expected message. A single parametrized test keeps the intent while aligning with the test guidelines.
Suggested refactor
-@pytest.mark.usefixtures("data_migration_file") -def test_migrate_data_single_migration_not_found(runner): - result = runner.invoke(app, ["migrate", "--data", "not_existing_migration"]) - - assert result.exit_code == 1, result.output - assert "Error running data command: Migration not_existing_migration not found" in result.output - - -@pytest.mark.usefixtures("data_migration_file", "schema_migration_file") -def test_migrate_data_single_migration_wrong_type(runner): - result = runner.invoke(app, ["migrate", "--data", "fake_schema_file_name"]) - - assert result.exit_code == 1, result.output - assert ( - "Error running data command: Migration fake_schema_file_name is not a data migration" - in result.output - ) +@pytest.mark.usefixtures("data_migration_file", "schema_migration_file") +@pytest.mark.parametrize( + ("migration_id", "expected_message"), + [ + ( + "not_existing_migration", + "Error running data command: Migration not_existing_migration not found", + ), + ( + "fake_schema_file_name", + "Error running data command: Migration fake_schema_file_name is not a data migration", + ), + ], +) +def test_migrate_data_single_migration_error_cases(runner, migration_id, expected_message): + result = runner.invoke(app, ["migrate", "--data", migration_id]) + + assert result.exit_code == 1, result.output + assert expected_message in result.outputAs per coding guidelines: "Use
@pytest.mark.parametrizewhen testing permutations of the same behavior" and "Avoid duplicating test logic; extract shared setup into fixtures".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli/test_cli_local_storage.py` around lines 124 - 141, Replace the two duplicate tests test_migrate_data_single_migration_not_found and test_migrate_data_single_migration_wrong_type with a single parametrized test that accepts migration_id and expected_message, keep the shared `@pytest.mark.usefixtures`("data_migration_file") plus add "schema_migration_file" only for the case that needs it (or include both fixtures if simpler), call runner.invoke(app, ["migrate", "--data", migration_id]) inside the test, and assert result.exit_code == 1 and that expected_message is in result.output; reference the existing test names and the runner.invoke(app, ["migrate", "--data", ...]) call to locate where to consolidate the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/cli/test_cli_local_storage.py`:
- Around line 54-73: Combine the duplicate assertions by parameterizing the
tests: replace test_migrate_data_single_migration and the existing full-data
migration test with a single parametrized test (e.g.,
test_migrate_data_variants) that takes the CLI args (like
["migrate","--data","fake_data_file_name"] and the full-data args) and the
expected migration_id/order_id/started_at/applied_at; use
`@pytest.mark.parametrize` to pass both argument sets and reuse the same fixtures
(migration_state_file, runner, log) and assertions currently inside
test_migrate_data_single_migration (including checking result.exit_code,
migration_state_file JSON contents, and log/output strings) so you avoid
duplicate assertions and keep shared setup in the fixtures.
- Around line 124-141: Replace the two duplicate tests
test_migrate_data_single_migration_not_found and
test_migrate_data_single_migration_wrong_type with a single parametrized test
that accepts migration_id and expected_message, keep the shared
`@pytest.mark.usefixtures`("data_migration_file") plus add "schema_migration_file"
only for the case that needs it (or include both fixtures if simpler), call
runner.invoke(app, ["migrate", "--data", migration_id]) inside the test, and
assert result.exit_code == 1 and that expected_message is in result.output;
reference the existing test names and the runner.invoke(app, ["migrate",
"--data", ...]) call to locate where to consolidate the assertions.
c26a494 to
ecbb422
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
mpt_tool/services/migration_state.py (1)
26-33: Adding a default case is a good defensive coding practice, but not required by current usage.The
save_statemethod is only called withRUNNING,FAILED, andAPPLIEDstatuses in the codebase. The other enum values (MANUAL_APPLIEDandNOT_APPLIED) are read-only states derived from the database and never passed to this method. However, a default case would make the intent explicit and guard against accidental misuse if the method's contract changes.Suggested defensive addition
match status: case MigrationStatusEnum.APPLIED: state.applied() case MigrationStatusEnum.FAILED: state.failed() case MigrationStatusEnum.RUNNING: state.start() + case _: + pass # Only handle operational state transitions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_tool/services/migration_state.py` around lines 26 - 33, The match in save_state currently handles MigrationStatusEnum.APPLIED, FAILED, and RUNNING by calling state.applied(), state.failed(), and state.start(); add a default branch to make the intent explicit and guard against misuse—e.g., add a case _ (or default) that either raises a clear exception (ValueError/TypeError) mentioning the unexpected MigrationStatusEnum value or logs and no-ops, so unexpected values like MANUAL_APPLIED or NOT_APPLIED are rejected/handled explicitly in save_state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/use_cases/test_migration_state.py`:
- Around line 43-59: The test currently patches Migration methods using
mocker.patch.object without autospec, which can allow invalid attribute access;
update the patch call in test_save_state_applied to use autospec=True when
patching Migration (e.g., mocker.patch.object(Migration, expected_method_call,
autospec=True)) so the mock matches the real Migration method signature and
respects the spec; keep the rest of the test intact (state_manager =
mocker.Mock(spec=StateManager), service = MigrationStateService(...),
service.save_state(...), and the same assertions).
- Around line 15-25: The test test_get_or_create_state_existing incorrectly uses
mocker.Mock(spec=mock_state); replace the mock spec with the StateManager type
expected by MigrationStateService and configure the mock's get_by_id to return
the mock_state fixture so MigrationStateService.get_or_create_state can call
state_manager.get_by_id; ensure the mock is created as state_manager =
mocker.Mock(spec=StateManager) (or the StateManager class used in the code), set
state_manager.get_by_id.return_value = mock_state, and keep assertions on
state_manager.get_by_id.assert_called_once_with("fake_id") and
state_manager.new.assert_not_called().
---
Nitpick comments:
In `@mpt_tool/services/migration_state.py`:
- Around line 26-33: The match in save_state currently handles
MigrationStatusEnum.APPLIED, FAILED, and RUNNING by calling state.applied(),
state.failed(), and state.start(); add a default branch to make the intent
explicit and guard against misuse—e.g., add a case _ (or default) that either
raises a clear exception (ValueError/TypeError) mentioning the unexpected
MigrationStatusEnum value or logs and no-ops, so unexpected values like
MANUAL_APPLIED or NOT_APPLIED are rejected/handled explicitly in save_state.
ecbb422 to
263598a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_tool/cli.py`:
- Around line 54-56: The migration_id argument is flagged as unused by Ruff
(ARG001) because it's only read via ctx.params; add a no-op reference to
migration_id inside the function body (e.g., assign it to underscore or call it
in a comment-safe noop) to mark it as used so the linter is satisfied; update
the function that declares migration_id (reference symbol migration_id and the
ctx.params usage) to include that single-line no-op.
263598a to
f643114
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/services/test_migration_state.py (1)
52-60: Test function name is misleading.The test
test_save_state_appliedis parameterized to test all three status transitions (APPLIED,FAILED,RUNNING), not justAPPLIED. Consider renaming totest_save_stateortest_save_state_transitionsfor clarity.♻️ Proposed fix
-def test_save_state_applied(status, expected_method_call, mocker, mock_state): +def test_save_state(status, expected_method_call, mocker, mock_state):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/test_migration_state.py` around lines 52 - 60, Rename the misleading test function test_save_state_applied to a more accurate name like test_save_state_transitions (or test_save_state) since it is parameterized over multiple statuses (APPLIED, FAILED, RUNNING); update the test function identifier and any references in the test file or test markers accordingly, leaving the body unchanged (calls to MigrationStateService.save_state, the mocked Migration method via mocker.patch.object, and assertions on mock_method.assert_called_once() and state_manager.save_state.assert_called_once_with(mock_state) should remain intact).mpt_tool/services/migration_state.py (1)
24-34: Match statement doesn't handle allMigrationStatusEnumvalues.The
matchstatement only handlesAPPLIED,FAILED, andRUNNING, butMigrationStatusEnumalso includesMANUAL_APPLIEDandNOT_APPLIED. If one of those is passed, no transition is applied butsave_statestill executes, which could lead to silent bugs.Consider adding a catch-all case that raises an error for unsupported statuses:
♻️ Proposed fix
def save_state(self, state: Migration, status: MigrationStatusEnum) -> None: """Apply status transition and persist migration state.""" match status: case MigrationStatusEnum.APPLIED: state.applied() case MigrationStatusEnum.FAILED: state.failed() case MigrationStatusEnum.RUNNING: state.start() + case _: + raise ValueError(f"Unsupported status: {status}") self._state_manager.save_state(state)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_tool/services/migration_state.py` around lines 24 - 34, The match in save_state (in MigrationState.save_state) only handles MigrationStatusEnum.APPLIED/FAILED/RUNNING; update it to explicitly handle the remaining enum values (MigrationStatusEnum.MANUAL_APPLIED and MigrationStatusEnum.NOT_APPLIED) by invoking the appropriate transition methods on the Migration instance (or, if there are no transitions, document that and do nothing), and add a catch-all case that raises a clear exception (e.g., ValueError) for any unsupported MigrationStatusEnum to avoid silently persisting an unchanged state; reference the save_state function, MigrationStatusEnum, and the Migration methods (state.applied, state.failed, state.start, and any manual/none transition methods) when making the change.mpt_tool/use_cases/run_single_migration.py (1)
17-25: Use theFileMigrationManagerclass directly instead of instantiating it.
FileMigrationManageruses@classmethodfor all its methods and has no instance state. Instantiating it unnecessarily creates overhead while calling classmethods on the instance. Update the parameter type totype[FileMigrationManager]and pass the class itself:def __init__( self, file_migration_manager: type[FileMigrationManager] | None = None, state_manager: StateManager | None = None, state_service: MigrationStateService | None = None, ): self.file_migration_manager = file_migration_manager or FileMigrationManager self.state_manager = state_manager or StateManagerFactory.get_instance() self.state_service = state_service or MigrationStateService(self.state_manager)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_tool/use_cases/run_single_migration.py` around lines 17 - 25, The constructor is instantiating FileMigrationManager even though all its methods are classmethods; change the parameter and assignment in __init__ to accept and store the class itself (type[FileMigrationManager]) instead of an instance: replace the file_migration_manager parameter type and default to FileMigrationManager (not FileMigrationManager()), and assign self.file_migration_manager = file_migration_manager or FileMigrationManager; leave state_manager (StateManagerFactory.get_instance()) and state_service (MigrationStateService(self.state_manager)) logic unchanged so callers can still inject those dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mpt_tool/cli.py`:
- Around line 54-56: The migration_id parameter is flagged as unused (ARG001)
even though ctx.params reads it later; add an explicit no-op reference to
migration_id inside the CLI handler to satisfy the linter (for example assign it
to _ or call a trivial passthrough) while leaving ctx.params usage
unchanged—locate the parameter named migration_id and add the single-line no-op
near the start of the function that also references ctx.params to silence
ARG001.
---
Nitpick comments:
In `@mpt_tool/services/migration_state.py`:
- Around line 24-34: The match in save_state (in MigrationState.save_state) only
handles MigrationStatusEnum.APPLIED/FAILED/RUNNING; update it to explicitly
handle the remaining enum values (MigrationStatusEnum.MANUAL_APPLIED and
MigrationStatusEnum.NOT_APPLIED) by invoking the appropriate transition methods
on the Migration instance (or, if there are no transitions, document that and do
nothing), and add a catch-all case that raises a clear exception (e.g.,
ValueError) for any unsupported MigrationStatusEnum to avoid silently persisting
an unchanged state; reference the save_state function, MigrationStatusEnum, and
the Migration methods (state.applied, state.failed, state.start, and any
manual/none transition methods) when making the change.
In `@mpt_tool/use_cases/run_single_migration.py`:
- Around line 17-25: The constructor is instantiating FileMigrationManager even
though all its methods are classmethods; change the parameter and assignment in
__init__ to accept and store the class itself (type[FileMigrationManager])
instead of an instance: replace the file_migration_manager parameter type and
default to FileMigrationManager (not FileMigrationManager()), and assign
self.file_migration_manager = file_migration_manager or FileMigrationManager;
leave state_manager (StateManagerFactory.get_instance()) and state_service
(MigrationStateService(self.state_manager)) logic unchanged so callers can still
inject those dependencies.
In `@tests/services/test_migration_state.py`:
- Around line 52-60: Rename the misleading test function test_save_state_applied
to a more accurate name like test_save_state_transitions (or test_save_state)
since it is parameterized over multiple statuses (APPLIED, FAILED, RUNNING);
update the test function identifier and any references in the test file or test
markers accordingly, leaving the body unchanged (calls to
MigrationStateService.save_state, the mocked Migration method via
mocker.patch.object, and assertions on mock_method.assert_called_once() and
state_manager.save_state.assert_called_once_with(mock_state) should remain
intact).
f643114 to
da3d0cc
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli/test_cli_local_storage.py (1)
124-139: Parameterize the single-migration error cases to reduce duplication.
These two tests are permutations of the same failure path and can be consolidated.♻️ Proposed refactor
-@pytest.mark.usefixtures("data_migration_file") -def test_migrate_data_single_migration_not_found(runner): - result = runner.invoke(app, ["migrate", "--data", "not_existing_migration"]) - - assert result.exit_code == 1, result.output - assert "Error running data command: Migration not_existing_migration not found" in result.output - - -@pytest.mark.usefixtures("data_migration_file", "schema_migration_file") -def test_migrate_data_single_migration_wrong_type(runner): - result = runner.invoke(app, ["migrate", "--data", "fake_schema_file_name"]) - - assert result.exit_code == 1, result.output - assert ( - "Error running data command: Migration fake_schema_file_name is not a data migration" - in result.output - ) +@pytest.mark.usefixtures("data_migration_file", "schema_migration_file") +@pytest.mark.parametrize( + ("args", "expected_message"), + [ + ( + ["migrate", "--data", "not_existing_migration"], + "Error running data command: Migration not_existing_migration not found", + ), + ( + ["migrate", "--data", "fake_schema_file_name"], + "Error running data command: Migration fake_schema_file_name is not a data migration", + ), + ], +) +def test_migrate_data_single_migration_errors(runner, args, expected_message): + result = runner.invoke(app, args) + + assert result.exit_code == 1, result.output + assert expected_message in result.outputAs per coding guidelines: "Use
@pytest.mark.parametrizewhen testing permutations of the same behavior" and "Avoid duplicating test logic; extract shared setup into fixtures".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli/test_cli_local_storage.py` around lines 124 - 139, The two tests test_migrate_data_single_migration_not_found and test_migrate_data_single_migration_wrong_type duplicate the same failure path; replace them with a single parametrized test using pytest.mark.parametrize to iterate over the migration name and expected error substring, keeping the shared fixtures (data_migration_file and schema_migration_file) applied as needed; update the test function (e.g., test_migrate_data_single_migration_errors) to call runner.invoke(app, ["migrate", "--data", migration_name]), assert exit_code == 1 and assert the expected error message substring in result.output for each case so the setup and assertions are not duplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/cli/test_cli_local_storage.py`:
- Around line 124-139: The two tests
test_migrate_data_single_migration_not_found and
test_migrate_data_single_migration_wrong_type duplicate the same failure path;
replace them with a single parametrized test using pytest.mark.parametrize to
iterate over the migration name and expected error substring, keeping the shared
fixtures (data_migration_file and schema_migration_file) applied as needed;
update the test function (e.g., test_migrate_data_single_migration_errors) to
call runner.invoke(app, ["migrate", "--data", migration_name]), assert exit_code
== 1 and assert the expected error message substring in result.output for each
case so the setup and assertions are not duplicated.
svazquezco
force-pushed
the
MPT-18132-add-command-to-run-a-single-migration
branch
from
da3d0cc to
0b5c7cd
Compare
|
d3rky
approved these changes
Feb 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes MPT-18132
Release Notes