Test: start a test suite for migration utils functions#8225
Open
CarolineDenis wants to merge 79 commits into
Open
Test: start a test suite for migration utils functions#8225CarolineDenis wants to merge 79 commits into
CarolineDenis wants to merge 79 commits into
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds comprehensive unit tests for migration utilities and management command, extracts discipline resource directory deduplication to a dedicated module, and consolidates migration helper imports. The test suite spans bulk creation, schema operations, default data initialization, deduplication, and tectonic ranks. Integration tests verify the migration command is idempotent across multiple runs without creating duplicates. ChangesMigration Utils Unit Test Suite
Management Command Test Infrastructure and Integration Tests
Deduplication Helper Extraction and Import Updates
Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
specifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.py (1)
63-63:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSame issue: passing
_mock_new_parentinstead of a proper apps mock.This line has the same problem as line 36. Use a proper mock apps object.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.py` at line 63, The call to create_cotype_splocalecontaineritem is passing mock_container._mock_new_parent as the apps argument, but this should be a proper mock apps object instead. Replace the _mock_new_parent argument with a properly constructed mock apps object (following the same pattern used to fix the similar issue at line 36) to ensure the test passes the correct type of dependency to the function.
🧹 Nitpick comments (1)
specifyweb/specify/migration_utils/tests/test_schema_reader.py (1)
122-122: 💤 Low valueRemove trailing blank line.
The file ends with an extra blank line after the closing of the
if __name__ == '__main__'block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/migration_utils/tests/test_schema_reader.py` at line 122, There is an extra blank line at the end of the file after the if __name__ == '__main__' block. Remove the trailing blank line at the end of the test_schema_reader.py file to clean up the file formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/specify/migration_utils/tests/test_bulk_create.py`:
- Around line 46-50: The test is named with "idempotent" but doesn't actually
verify idempotent behavior. After the first assertion where result equals 2, add
a second call to the same function being tested using the same input rows, then
assert that this second call returns 0 to confirm that no duplicate rows are
created. This verifies that the function properly checks for existing rows and
avoids redundant bulk_create operations when called with identical data.
- Around line 16-22: The mock queryset setup in the test does not properly
simulate the chained filter calls used in the real function. The real
implementation calls filter twice in succession before order_by, but the current
mock setup only chains filter once. Modify the mock_qs setup so that the first
filter call returns an object whose filter method can be called again, and that
second filter call returns an object with an order_by method. Specifically,
ensure mock_qs.filter.return_value has its own filter method configured to
return an object that has order_by chained properly, allowing the double filter
pattern to work correctly.
- Around line 11-12: The `@patch` decorator is replacing the actual
bulk_create_splocaleitemstr_idempotent function with a mock, preventing the test
from verifying the real implementation logic. Remove the `@patch` decorator
entirely and instead mock only the internal ORM dependencies (like
model.objects.filter and model.objects.bulk_create) that the actual function
uses. Update the test assertions to verify that the real function correctly
calls these mocked ORM methods with the expected parameters (FK filters,
language filters, and bulk_create calls), rather than asserting on the mock of
the function itself.
In
`@specifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.py`:
- Line 36: The function create_cotype_splocalecontaineritem is being called with
mock_container._mock_new_parent, which is an internal MagicMock attribute and
not a valid apps object with the required get_model method. Create a proper mock
apps object that implements the get_model(app_label, model_name) method as
expected by the function signature, then pass this mock apps object as the
argument to the create_cotype_splocalecontaineritem call instead of the
_mock_new_parent attribute.
- Around line 9-11: The test is patching module-level names that don't actually
exist in helper_0003_cotype_picklist.py, but the real
create_cotype_splocalecontaineritem function dynamically obtains models using
apps.get_model('specify', ModelName). Replace the three `@patch` decorators
targeting Splocalecontainer, Splocalecontaineritem, and Splocaleitemstr with a
single `@patch` for the apps parameter, then configure apps.get_model.side_effect
to return the appropriate mock models based on the model name argument passed to
it. Apply this same pattern to both test methods in the file.
In `@specifyweb/specify/migration_utils/tests/test_schema_reader.py`:
- Around line 28-48: The test assertion for mock_path is incorrect because it
expects the full concatenated path string, but the actual function constructs
the path using chained division operators starting with
Path(settings.SPECIFY_CONFIG_DIR). Update the test to properly mock the Path
chaining behavior by setting up mock_path.return_value to support the
__truediv__ method (which implements the / operator) and chain operations, then
correct the assertion to expect mock_path to be called once with just "/config"
instead of the full path string. The mock path instance should also have
exists() and open() methods properly configured to support the chaining pattern.
In `@specifyweb/specify/migration_utils/tests/test_schema_writer.py`:
- Around line 12-16: The test patches are targeting non-existent module-level
imports in schema_writer, but the real
`update_table_schema_config_with_defaults` function obtains models through
`apps.get_model()` calls, not from direct imports. Instead of patching
`Splocalecontainer`, `Splocalecontaineritem`, `Splocaleitemstr` as if they were
module-level imports, create a mock `apps` object, configure its `get_model()`
method to return the appropriate mock models for each model name (specify,
Splocalecontainer), (specify, Splocalecontaineritem), and (specify,
Splocaleitemstr), and pass this mock as a parameter to the
`update_table_schema_config_with_defaults` function call in the test. Also patch
the `apps` import in the schema_writer module itself.
- Around line 65-72: The test is patching module-level names that the function
`revert_table_schema_config` does not use. Instead, the function obtains models
via `apps.get_model()`, but the test calls
`revert_table_schema_config("TestTable")` without passing an `apps` argument,
causing it to use the real Django registry (global_apps). Remove the three
`@patch` decorators for Splocalecontainer, Splocaleitemstr, and
Splocalecontaineritem. Instead, create a mock `apps` object that is configured
to return the mocked model instances when `get_model()` is called with the
appropriate model names, and pass this mock `apps` object as a parameter to the
`revert_table_schema_config` function call.
---
Duplicate comments:
In
`@specifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.py`:
- Line 63: The call to create_cotype_splocalecontaineritem is passing
mock_container._mock_new_parent as the apps argument, but this should be a
proper mock apps object instead. Replace the _mock_new_parent argument with a
properly constructed mock apps object (following the same pattern used to fix
the similar issue at line 36) to ensure the test passes the correct type of
dependency to the function.
---
Nitpick comments:
In `@specifyweb/specify/migration_utils/tests/test_schema_reader.py`:
- Line 122: There is an extra blank line at the end of the file after the if
__name__ == '__main__' block. Remove the trailing blank line at the end of the
test_schema_reader.py file to clean up the file formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 09e00975-8c37-4bc2-91a7-45fae919b02c
📒 Files selected for processing (5)
specifyweb/specify/migration_utils/tests/__init__.pyspecifyweb/specify/migration_utils/tests/test_bulk_create.pyspecifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.pyspecifyweb/specify/migration_utils/tests/test_schema_reader.pyspecifyweb/specify/migration_utils/tests/test_schema_writer.py
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
…list for COG type business rule
Base automatically changed from
issue-8058
to
issue-8124-8126-fix-7660-7682-2-after-review-2
June 24, 2026 17:59
This was
linked to
issues
Jun 25, 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.
Fixes #8224
Fixes #8049
Fixes #8042
Checklist
self-explanatory (or properly documented)
specify7/specifyweb/specify/management/commands/run_key_migration_functions.py
Line 50 in ea04665
Testing instructions
Summary by CodeRabbit
New Features
Bug Fixes
selectseriesdefaults are consistently enforced toFalseduring migrations.Tests