Test: start a test suite for migration utils functions#8225
Open
CarolineDenis wants to merge 93 commits into
Open
Test: start a test suite for migration utils functions#8225CarolineDenis wants to merge 93 commits into
CarolineDenis wants to merge 93 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:
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:
📝 WalkthroughWalkthroughThis PR adds extensive Django/unittest coverage for ChangesMigration Utils Unit Test Suite
Management Command Test Infrastructure and Integration Tests
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ 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.
|
Base automatically changed from
issue-8124-8126-fix-7660-7682-2-after-review-2
to
v7_12_0_7_base
June 30, 2026 12:42
added 7 commits
June 30, 2026 14:50
Contributor
Author
|
@CodeRabbit full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
specifyweb/specify/management/commands/tests/business_rules_tests.py (1)
66-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest does not cover the multi-field exclusion branch of
catnum_rule_editable.The upstream helper (
specifyweb/backend/businessrules/migration_utils.py) explicitly guards against rules with more than one scope/field vialen(fields) == 1 and len(scopes) == 1, since.get()raises if more than one match. This test only covers exact-match and different-field-path cases, leaving the multi-field guard unverified.♻️ Suggested additional case
+ multi_field_rule = UniquenessRule.objects.create( + modelName="Collectionobject", + discipline=discipline, + isDatabaseConstraint=True, + ) + UniquenessRuleField.objects.create( + uniquenessrule=multi_field_rule, + fieldPath="catalogNumber", + isScope=False, + ) + UniquenessRuleField.objects.create( + uniquenessrule=multi_field_rule, + fieldPath="collection", + isScope=True, + ) + UniquenessRuleField.objects.create( + uniquenessrule=multi_field_rule, + fieldPath="collectionobjecttype", + isScope=True, + ) + rkm.catnum_rule_editable(django_apps) matching_rule.refresh_from_db() nonmatching_rule.refresh_from_db() + multi_field_rule.refresh_from_db() self.assertFalse(matching_rule.isDatabaseConstraint) self.assertTrue(nonmatching_rule.isDatabaseConstraint) + self.assertTrue(multi_field_rule.isDatabaseConstraint)🤖 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/management/commands/tests/business_rules_tests.py` around lines 66 - 105, The `BusinessRulesDatabaseTests.test_catnum_rule_editable_only_updates_matching_catalog_number_rule` test is missing coverage for the multi-field exclusion branch in `catnum_rule_editable`. Add a case that creates a `UniquenessRule` with more than one non-scope field or more than one scope field, then call `rkm.catnum_rule_editable(django_apps)` and assert that this rule remains unchanged. Use the existing `UniquenessRule` and `UniquenessRuleField` setup patterns so the test verifies the `len(fields) == 1 and len(scopes) == 1` guard in `migration_utils`.specifyweb/specify/migration_utils/tests/test_schema_reader.py (1)
120-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFinish or drop the commented-out
find_missing_schema_config_fieldstest.This test is fully disabled behind a
#TODO, leavingfind_missing_schema_config_fields(imported at line 14 but otherwise untested) without coverage — one of the utilities this PR's objectives call out for testing.Want me to flesh out this test (fixing the
mock_table._all_fieldsreturn value to use real field-name attributes matching the actual signature) or should this be tracked as a follow-up issue?🤖 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` around lines 120 - 159, Either restore the disabled find_missing_schema_config_fields test or remove the dead scaffold entirely; the current `#TODO` leaves this utility untested despite being imported and intended for coverage. If you keep it, update test_find_missing_schema_config_fields to match the real schema_reader/find_missing_schema_config_fields behavior, including mocking datamodel and global_apps correctly and using _all_fields values that expose the actual field-name attribute expected by the implementation.specifyweb/specify/migration_utils/tests/test_helper_0039_agent_fields_for_loan_and_gift.py (1)
14-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded call count is fragile vs. sibling tests.
10implicitly assumes exactly oneDisciplinefixture row. Sibling tests in this cohort (e.g.test_helper_0004_stratigraphy_age.py,test_helper_0008_ageCitations_fix.py,test_helper_0012_add_cojo_to_schema_config.py) compute the expected count dynamically fromDiscipline.objects.count()× field count, which is more resilient to fixture changes.♻️ Proposed fix for consistency
+from specifyweb.specify.models import Discipline + def test_calls_schema_writer_for_all_fields(self, mock_update): helper.update_loan_and_gift_agent_fields(django_apps) - self.assertEqual(mock_update.call_count, 10) + expected = Discipline.objects.count() * sum( + len(fields) for fields in helper.MIGRATION_0039_FIELDS.values() + ) + self.assertEqual(mock_update.call_count, expected)🤖 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_0039_agent_fields_for_loan_and_gift.py` around lines 14 - 21, The test in test_calls_schema_writer_for_all_fields is hardcoding the expected update count, which makes it brittle if fixture data changes. Update the assertion to derive the expected number of calls from the current Discipline.objects.count() multiplied by the number of fields updated by helper.update_loan_and_gift_agent_fields, matching the pattern used in the sibling migration helper tests. Keep the kwargs["defaults"] check as-is to verify each schema update remains hidden.specifyweb/specify/migration_utils/tests/test_helper_0040_components.py (1)
93-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
container+name="type"items in setUp.
self.component_type_item(Line 26-31) andself.hidden_field(Line 100-105) both usecontainer=self.component_container, name="type". Filters likecreate_cotype_splocalecontaineritem_for_componentsmatch oncontainer+nameand will affect both rows via.update(), which happens to not break current assertions, but it's fragile if a future helper expects a single unique row per container/name.🤖 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_0040_components.py` around lines 93 - 105, `TestHelper0040Components.setUp` creates two Splocalecontaineritem rows with the same `container=self.component_container` and `name="type"`, which makes later helpers like `create_cotype_splocalecontaineritem_for_components` ambiguous because they update by container+name. Change the test fixture so only one `type` item exists per component container, or use a different name for `hidden_field`, and keep the existing references (`self.component_type_item`, `self.hidden_field`) aligned with the intended uniqueness.specifyweb/specify/migration_utils/tests/test_tectonic_ranks.py (1)
209-271: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen assertions on constructed
Tectonicunittreedefobjects.
test_creates_missing_tree_defs_and_updates_discipline_linksonly checkslen(bulk_create_args) == 3and re-assertsbulk_createwas called with the same list object it just captured (a tautology that can't catch a regression). It doesn't verify that each constructed model instance actually received the correctdiscipline_id(1, 2, 3) andname="Tectonic Unit". SinceTectonicTreeDef_Modelis aMagicMock, itscall_args_listcan be inspected directly to catch wrong-field regressions in_create_tectonic_unit_for_discipline.♻️ Suggested strengthening
TectonicTreeDef_Model.objects.bulk_create.assert_called_once_with( bulk_create_args, batch_size=1000, ) + + TectonicTreeDef_Model.assert_has_calls( + [ + call(name="Tectonic Unit", discipline_id=disciplineid) + for disciplineid in missing_disciplines + ] + )🤖 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_tectonic_ranks.py` around lines 209 - 271, The test only checks list length and reuses the same bulk_create argument, so it misses regressions in how `_create_tectonic_unit_for_discipline` builds `Tectonicunittreedef` instances. Update `test_creates_missing_tree_defs_and_updates_discipline_links` to inspect `TectonicTreeDef_Model.call_args_list` (or the created objects passed to `bulk_create`) and assert each instance was constructed with the expected `discipline_id` values 1, 2, and 3 and `name="Tectonic Unit"`. Keep the existing `bulk_create` and update assertions, but make the object-field checks the core validation.specifyweb/specify/management/commands/tests/tectonic_tests.py (1)
36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStatic analysis: add
strict=tozip().Ruff flags this
zip()call without an explicitstrict=parameter (B905). Hereitemsanditems[1:]always differ by exactly one element by construction, so there's no real truncation risk, but addingstrict=False(or usingitertools.pairwise(items)) satisfies the linter and documents intent.🧹 Suggested fix
- for parent, child in zip(items, items[1:]): + for parent, child in zip(items, items[1:], strict=False): self.assertEqual(child.parent_id, parent.id)🤖 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/management/commands/tests/tectonic_tests.py` around lines 36 - 37, Add an explicit strict argument to the zip in the test loop that iterates over items and items[1:] so Ruff B905 is satisfied. Update the parent/child iteration in the tectonic test to use zip(..., strict=False), or switch to itertools.pairwise in that test helper if you prefer clearer intent, keeping the assertion on child.parent_id unchanged.Source: Linters/SAST tools
specifyweb/specify/management/commands/tests/test_run_key_migration_functions.py (1)
26-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTRACKED_MODELS omits models created by
simulate_specify7_usage.
Collectionobject,Spappresource, andSpappresourcedataare created insimulate_specify7_usage(lines 111-177) but aren't tracked inTRACKED_MODELS. If a migration section were to accidentally duplicate or drop any of these on a rerun,count_diffwouldn't detect it — weakening the idempotency guarantee this test is meant to prove.🤖 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/management/commands/tests/test_run_key_migration_functions.py` around lines 26 - 46, TRACKED_MODELS is missing several models that `simulate_specify7_usage` creates, so the idempotency check in `count_diff` can miss duplicate or dropped rows. Add the missing tracked entries for `Collectionobject`, `Spappresource`, and `Spappresourcedata` to `TRACKED_MODELS` so the test covers all models touched by `simulate_specify7_usage`.
🤖 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/management/commands/tests/business_rules_tests.py`:
- Line 68: Populate the required fields when creating the test Discipline in
business_rules_tests instead of using only name. Update the
Discipline.objects.create call in the test setup to supply division, datatype,
geographytreedef, and geologictimeperiodtreedef so the fixture can be created
successfully before reaching catnum_rule_editable.
In `@specifyweb/specify/management/commands/tests/schema_config_tests.py`:
- Around line 31-77: The test’s ordered helper list in schema_config_tests is
missing update_loan_and_gift_agent_fields, so it is not being patched and the
real helper can run against FakeApps. Add update_loan_and_gift_agent_fields to
the names sequence in the same position used by fix_schema_config, and keep the
expected calls assertion in sync so the recorder order matches the actual
workflow.
In
`@specifyweb/specify/management/commands/tests/test_run_key_migration_functions_order.py`:
- Around line 20-48: This test class duplicates helper logic that already exists
in MigrationCommandTestCase, so refactor KeyMigrationSectionTests to inherit
from that base instead of redefining _recorder, _patch_recorders, and
_assert_section_calls. Keep the test body focused on key migration ordering and
rely on the shared helpers from MigrationCommandTestCase, matching the approach
used by command_tests.py. Remove the copied helper methods and update the class
hierarchy so the shared behavior is reused from the existing base test class.
In
`@specifyweb/specify/management/commands/tests/test_run_key_migration_functions.py`:
- Around line 374-386: The first-run backfill behavior is not being verified
because first_run_diff is computed in the run_key_migration_functions test but
never asserted. Update this test to make a meaningful assertion about the
initial change in record_counts/count_diff after the first call to
run_key_migration_functions, using first_run_diff to confirm the expected
migration records were created or backfilled on the first run.
In `@specifyweb/specify/migration_utils/tests/test_helper_0040_components.py`:
- Around line 195-202: The test_reverse_hide_component_fields assertion is
inverted: after calling reverse_hide_component_fields(apps), the hidden_field
should be verified as unhidden. Update the final assertion in
test_reverse_hide_component_fields so it checks the refreshed
self.hidden_field.ishidden is False, matching the behavior of
helper_0040_components.reverse_hide_component_fields.
In
`@specifyweb/specify/migration_utils/tests/test_helper_0042_discipline_type_picklist.py`:
- Around line 135-147: The revert test in
DisciplineTypeSplocaleContainerItemRevertTests does not exercise the migration
logic because it never creates the matching
Splocalecontainer/Splocalecontaineritem fixture and only asserts
Collection.objects.all().exists(), which is unrelated. Update
test_revert_splocalecontaineritem to mirror the sibling update test by creating
the exact filtered fixture before calling
helper_0042_discipline_type_picklist.update_discipline_type_splocalecontaineritem
and
helper_0042_discipline_type_picklist.revert_discipline_type_splocalecontaineritem,
then assert on the reverted Splocalecontaineritem field state rather than
collection existence.
---
Nitpick comments:
In `@specifyweb/specify/management/commands/tests/business_rules_tests.py`:
- Around line 66-105: The
`BusinessRulesDatabaseTests.test_catnum_rule_editable_only_updates_matching_catalog_number_rule`
test is missing coverage for the multi-field exclusion branch in
`catnum_rule_editable`. Add a case that creates a `UniquenessRule` with more
than one non-scope field or more than one scope field, then call
`rkm.catnum_rule_editable(django_apps)` and assert that this rule remains
unchanged. Use the existing `UniquenessRule` and `UniquenessRuleField` setup
patterns so the test verifies the `len(fields) == 1 and len(scopes) == 1` guard
in `migration_utils`.
In `@specifyweb/specify/management/commands/tests/tectonic_tests.py`:
- Around line 36-37: Add an explicit strict argument to the zip in the test loop
that iterates over items and items[1:] so Ruff B905 is satisfied. Update the
parent/child iteration in the tectonic test to use zip(..., strict=False), or
switch to itertools.pairwise in that test helper if you prefer clearer intent,
keeping the assertion on child.parent_id unchanged.
In
`@specifyweb/specify/management/commands/tests/test_run_key_migration_functions.py`:
- Around line 26-46: TRACKED_MODELS is missing several models that
`simulate_specify7_usage` creates, so the idempotency check in `count_diff` can
miss duplicate or dropped rows. Add the missing tracked entries for
`Collectionobject`, `Spappresource`, and `Spappresourcedata` to `TRACKED_MODELS`
so the test covers all models touched by `simulate_specify7_usage`.
In
`@specifyweb/specify/migration_utils/tests/test_helper_0039_agent_fields_for_loan_and_gift.py`:
- Around line 14-21: The test in test_calls_schema_writer_for_all_fields is
hardcoding the expected update count, which makes it brittle if fixture data
changes. Update the assertion to derive the expected number of calls from the
current Discipline.objects.count() multiplied by the number of fields updated by
helper.update_loan_and_gift_agent_fields, matching the pattern used in the
sibling migration helper tests. Keep the kwargs["defaults"] check as-is to
verify each schema update remains hidden.
In `@specifyweb/specify/migration_utils/tests/test_helper_0040_components.py`:
- Around line 93-105: `TestHelper0040Components.setUp` creates two
Splocalecontaineritem rows with the same `container=self.component_container`
and `name="type"`, which makes later helpers like
`create_cotype_splocalecontaineritem_for_components` ambiguous because they
update by container+name. Change the test fixture so only one `type` item exists
per component container, or use a different name for `hidden_field`, and keep
the existing references (`self.component_type_item`, `self.hidden_field`)
aligned with the intended uniqueness.
In `@specifyweb/specify/migration_utils/tests/test_schema_reader.py`:
- Around line 120-159: Either restore the disabled
find_missing_schema_config_fields test or remove the dead scaffold entirely; the
current `#TODO` leaves this utility untested despite being imported and intended
for coverage. If you keep it, update test_find_missing_schema_config_fields to
match the real schema_reader/find_missing_schema_config_fields behavior,
including mocking datamodel and global_apps correctly and using _all_fields
values that expose the actual field-name attribute expected by the
implementation.
In `@specifyweb/specify/migration_utils/tests/test_tectonic_ranks.py`:
- Around line 209-271: The test only checks list length and reuses the same
bulk_create argument, so it misses regressions in how
`_create_tectonic_unit_for_discipline` builds `Tectonicunittreedef` instances.
Update `test_creates_missing_tree_defs_and_updates_discipline_links` to inspect
`TectonicTreeDef_Model.call_args_list` (or the created objects passed to
`bulk_create`) and assert each instance was constructed with the expected
`discipline_id` values 1, 2, and 3 and `name="Tectonic Unit"`. Keep the existing
`bulk_create` and update assertions, but make the object-field checks the core
validation.
🪄 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: bf7768a4-f685-4e69-90ab-fc19ea9fe131
📒 Files selected for processing (49)
specifyweb/specify/management/commands/run_key_migration_functions.pyspecifyweb/specify/management/commands/tests/__init__.pyspecifyweb/specify/management/commands/tests/app_resource_tests.pyspecifyweb/specify/management/commands/tests/business_rules_tests.pyspecifyweb/specify/management/commands/tests/command_tests.pyspecifyweb/specify/management/commands/tests/cots_tests.pyspecifyweb/specify/management/commands/tests/permissions_tests.pyspecifyweb/specify/management/commands/tests/schema_config_tests.pyspecifyweb/specify/management/commands/tests/tectonic_tests.pyspecifyweb/specify/management/commands/tests/test_deduplicate_discipline.pyspecifyweb/specify/management/commands/tests/test_migration_base.pyspecifyweb/specify/management/commands/tests/test_run_key_migration_functions.pyspecifyweb/specify/management/commands/tests/test_run_key_migration_functions_order.pyspecifyweb/specify/migration_utils/migration_helpers/__init__.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0003_cotype_picklist.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0008_ageCitations_fix.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0017_schemaconfig_fixes.pyspecifyweb/specify/migration_utils/migration_helpers/helper_0031_add_default_for_selectseries.pyspecifyweb/specify/migration_utils/tests/__init__.pyspecifyweb/specify/migration_utils/tests/test_bulk_create.pyspecifyweb/specify/migration_utils/tests/test_deduplication.pyspecifyweb/specify/migration_utils/tests/test_default_cots.pyspecifyweb/specify/migration_utils/tests/test_helper_0003_cotype_picklist.pyspecifyweb/specify/migration_utils/tests/test_helper_0004_stratigraphy_age.pyspecifyweb/specify/migration_utils/tests/test_helper_0007_schema_config_update.pyspecifyweb/specify/migration_utils/tests/test_helper_0008_ageCitations_fix.pyspecifyweb/specify/migration_utils/tests/test_helper_0012_add_cojo_to_schema_config.pyspecifyweb/specify/migration_utils/tests/test_helper_0013_collectionobjectgroup_parentcog.pyspecifyweb/specify/migration_utils/tests/test_helper_0015_add_version_to_ages.pyspecifyweb/specify/migration_utils/tests/test_helper_0017_schemaconfig_fixes.pyspecifyweb/specify/migration_utils/tests/test_helper_0018_cot_catnum_schema.pyspecifyweb/specify/migration_utils/tests/test_helper_0020_add_tectonicunit_to_pc_in_schema_config.pyspecifyweb/specify/migration_utils/tests/test_helper_0021_update_hidden_geo_tables.pyspecifyweb/specify/migration_utils/tests/test_helper_0023_update_schema_config_text.pyspecifyweb/specify/migration_utils/tests/test_helper_0024_add_uniqueIdentifier_storage.pyspecifyweb/specify/migration_utils/tests/test_helper_0027_CO_children.pyspecifyweb/specify/migration_utils/tests/test_helper_0031_add_default_for_selectseries.pyspecifyweb/specify/migration_utils/tests/test_helper_0032_add_quantities_gift.pyspecifyweb/specify/migration_utils/tests/test_helper_0033_update_paleo_desc.pyspecifyweb/specify/migration_utils/tests/test_helper_0034_accession_date_fields.pyspecifyweb/specify/migration_utils/tests/test_helper_0035_version_required.pyspecifyweb/specify/migration_utils/tests/test_helper_0039_agent_fields_for_loan_and_gift.pyspecifyweb/specify/migration_utils/tests/test_helper_0040_components.pyspecifyweb/specify/migration_utils/tests/test_helper_0042_discipline_type_picklist.pyspecifyweb/specify/migration_utils/tests/test_schema_reader.pyspecifyweb/specify/migration_utils/tests/test_schema_writer.pyspecifyweb/specify/migration_utils/tests/test_tectonic_ranks.pyspecifyweb/specify/migrations/0008_ageCitations_fix.pyspecifyweb/specify/migrations/0031_add_default_for_selectseries.py
💤 Files with no reviewable changes (1)
- specifyweb/specify/migration_utils/migration_helpers/helper_0003_cotype_picklist.py
Comment on lines
+31
to
+77
| names = [ | ||
| # Ordered per fix_schema_config implementation | ||
| "create_geo_table_schema_config_with_defaults", | ||
| "create_cotype_splocalecontaineritem", | ||
| "create_strat_table_schema_config_with_defaults", | ||
| "create_agetype_picklist", | ||
| "create_cogtype_picklist", | ||
| "update_relative_age_fields", | ||
| "add_cojo_to_schema_config", | ||
| "update_cog_schema_config", | ||
| "update_age_schema_config", | ||
| "add_tectonicunit_to_pc_in_schema_config", | ||
| "update_storage_unique_id_fields", | ||
| "remove_componentparent_item", | ||
| "create_table_schema_config_with_defaults", | ||
| "create_discipline_type_picklist", | ||
| "apply_schema_overrides_for_all_disciplines", | ||
| "deduplicate_schema_config_orm", | ||
| ] | ||
| fake_apps = FakeApps() | ||
|
|
||
| with ExitStack() as stack: | ||
| stack.enter_context(patch.object(rkm, "apps", fake_apps)) | ||
| self._patch_recorders(stack, [(rkm.usc, name) for name in names], calls) | ||
| schema_defaults_apply_path = ( | ||
| "specifyweb.backend.setup_tool.schema_defaults." | ||
| "apply_schema_defaults_task.apply" | ||
| ) | ||
| stack.enter_context( | ||
| patch( | ||
| schema_defaults_apply_path, | ||
| apply_schema_defaults, | ||
| ) | ||
| ) | ||
|
|
||
| rkm.fix_schema_config(stdout) | ||
|
|
||
| self.assertEqual(fake_apps.model_request, ("specify", "Discipline")) | ||
| self.assertEqual( | ||
| calls, | ||
| [(name, fake_apps) for name in names[:-1]] | ||
| + [ | ||
| ("apply_schema_defaults_task.apply", [discipline_1.id]), | ||
| ("apply_schema_defaults_task.apply", [discipline_2.id]), | ||
| (names[-1], fake_apps), | ||
| ], | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Missing update_loan_and_gift_agent_fields from the names list breaks the test.
The real fix_schema_config funcs list includes update_loan_and_gift_agent_fields between update_storage_unique_id_fields and remove_componentparent_item, but it's absent from names here. Since only functions in names get patched via self._patch_recorders, the real unmocked helper runs against fake_apps (whose get_model returns a bare FakeDiscipline/SimpleNamespace lacking ORM methods) — likely raising an exception before the test can even reach its assertions, and breaking the exact-order calls assertion regardless.
🐛 Proposed fix
"update_storage_unique_id_fields",
+ "update_loan_and_gift_agent_fields",
"remove_componentparent_item",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| names = [ | |
| # Ordered per fix_schema_config implementation | |
| "create_geo_table_schema_config_with_defaults", | |
| "create_cotype_splocalecontaineritem", | |
| "create_strat_table_schema_config_with_defaults", | |
| "create_agetype_picklist", | |
| "create_cogtype_picklist", | |
| "update_relative_age_fields", | |
| "add_cojo_to_schema_config", | |
| "update_cog_schema_config", | |
| "update_age_schema_config", | |
| "add_tectonicunit_to_pc_in_schema_config", | |
| "update_storage_unique_id_fields", | |
| "remove_componentparent_item", | |
| "create_table_schema_config_with_defaults", | |
| "create_discipline_type_picklist", | |
| "apply_schema_overrides_for_all_disciplines", | |
| "deduplicate_schema_config_orm", | |
| ] | |
| fake_apps = FakeApps() | |
| with ExitStack() as stack: | |
| stack.enter_context(patch.object(rkm, "apps", fake_apps)) | |
| self._patch_recorders(stack, [(rkm.usc, name) for name in names], calls) | |
| schema_defaults_apply_path = ( | |
| "specifyweb.backend.setup_tool.schema_defaults." | |
| "apply_schema_defaults_task.apply" | |
| ) | |
| stack.enter_context( | |
| patch( | |
| schema_defaults_apply_path, | |
| apply_schema_defaults, | |
| ) | |
| ) | |
| rkm.fix_schema_config(stdout) | |
| self.assertEqual(fake_apps.model_request, ("specify", "Discipline")) | |
| self.assertEqual( | |
| calls, | |
| [(name, fake_apps) for name in names[:-1]] | |
| + [ | |
| ("apply_schema_defaults_task.apply", [discipline_1.id]), | |
| ("apply_schema_defaults_task.apply", [discipline_2.id]), | |
| (names[-1], fake_apps), | |
| ], | |
| ) | |
| names = [ | |
| # Ordered per fix_schema_config implementation | |
| "create_geo_table_schema_config_with_defaults", | |
| "create_cotype_splocalecontaineritem", | |
| "create_strat_table_schema_config_with_defaults", | |
| "create_agetype_picklist", | |
| "create_cogtype_picklist", | |
| "update_relative_age_fields", | |
| "add_cojo_to_schema_config", | |
| "update_cog_schema_config", | |
| "update_age_schema_config", | |
| "add_tectonicunit_to_pc_in_schema_config", | |
| "update_storage_unique_id_fields", | |
| "update_loan_and_gift_agent_fields", | |
| "remove_componentparent_item", | |
| "create_table_schema_config_with_defaults", | |
| "create_discipline_type_picklist", | |
| "apply_schema_overrides_for_all_disciplines", | |
| "deduplicate_schema_config_orm", | |
| ] | |
| fake_apps = FakeApps() | |
| with ExitStack() as stack: | |
| stack.enter_context(patch.object(rkm, "apps", fake_apps)) | |
| self._patch_recorders(stack, [(rkm.usc, name) for name in names], calls) | |
| schema_defaults_apply_path = ( | |
| "specifyweb.backend.setup_tool.schema_defaults." | |
| "apply_schema_defaults_task.apply" | |
| ) | |
| stack.enter_context( | |
| patch( | |
| schema_defaults_apply_path, | |
| apply_schema_defaults, | |
| ) | |
| ) | |
| rkm.fix_schema_config(stdout) | |
| self.assertEqual(fake_apps.model_request, ("specify", "Discipline")) | |
| self.assertEqual( | |
| calls, | |
| [(name, fake_apps) for name in names[:-1]] | |
| [ | |
| ("apply_schema_defaults_task.apply", [discipline_1.id]), | |
| ("apply_schema_defaults_task.apply", [discipline_2.id]), | |
| (names[-1], fake_apps), | |
| ], | |
| ) |
🤖 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/management/commands/tests/schema_config_tests.py` around
lines 31 - 77, The test’s ordered helper list in schema_config_tests is missing
update_loan_and_gift_agent_fields, so it is not being patched and the real
helper can run against FakeApps. Add update_loan_and_gift_agent_fields to the
names sequence in the same position used by fix_schema_config, and keep the
expected calls assertion in sync so the recorder order matches the actual
workflow.
Contributor
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
Comment on lines
+374
to
+386
| before_first_run = record_counts() | ||
|
|
||
| self.run_key_migration_functions( | ||
| "fix_schema_config", | ||
| "fix_app_resource_dirs", | ||
| "fix_permissions", | ||
| "fix_business_rules", | ||
| "fix_tectonic_ranks", | ||
| "fix_misc", | ||
| ) | ||
|
|
||
| after_first_run = record_counts() | ||
| first_run_diff = count_diff(before_first_run, after_first_run) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
first_run_diff is computed but never asserted — first-run backfill behavior is unverified.
Per issue #8042's objective, the test should verify the command "creates or backfills missing migration records on the first run." first_run_diff (line 386) is dead code — nothing checks that expected records were actually created/backfilled by the first run. As written, the test would pass even if the first run silently did nothing.
💡 Example: assert something meaningful about first_run_diff
after_first_run = record_counts()
first_run_diff = count_diff(before_first_run, after_first_run)
+ self.assertTrue(
+ first_run_diff,
+ "First run should create or backfill expected migration records",
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| before_first_run = record_counts() | |
| self.run_key_migration_functions( | |
| "fix_schema_config", | |
| "fix_app_resource_dirs", | |
| "fix_permissions", | |
| "fix_business_rules", | |
| "fix_tectonic_ranks", | |
| "fix_misc", | |
| ) | |
| after_first_run = record_counts() | |
| first_run_diff = count_diff(before_first_run, after_first_run) | |
| before_first_run = record_counts() | |
| self.run_key_migration_functions( | |
| "fix_schema_config", | |
| "fix_app_resource_dirs", | |
| "fix_permissions", | |
| "fix_business_rules", | |
| "fix_tectonic_ranks", | |
| "fix_misc", | |
| ) | |
| after_first_run = record_counts() | |
| first_run_diff = count_diff(before_first_run, after_first_run) | |
| self.assertTrue( | |
| first_run_diff, | |
| "First run should create or backfill expected migration records", | |
| ) |
🤖 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/management/commands/tests/test_run_key_migration_functions.py`
around lines 374 - 386, The first-run backfill behavior is not being verified
because first_run_diff is computed in the run_key_migration_functions test but
never asserted. Update this test to make a meaningful assertion about the
initial change in record_counts/count_diff after the first call to
run_key_migration_functions, using first_run_diff to confirm the expected
migration records were created or backfilled on the first run.
Contributor
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
Comment on lines
+195
to
+202
| def test_reverse_hide_component_fields(self): | ||
| self.hidden_field.ishidden = True | ||
| self.hidden_field.save() | ||
|
|
||
| helper_0040_components.reverse_hide_component_fields(apps) | ||
|
|
||
| self.hidden_field.refresh_from_db() | ||
| self.assertTrue(self.hidden_field.ishidden) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the reverse_hide_component_fields implementation to confirm expected ishidden value.
rg -n -B2 -A15 "def reverse_hide_component_fields" specifyweb/specify/migration_utils/migration_helpers/helper_0040_components.pyRepository: specify/specify7
Length of output: 1072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== implementation =="
sed -n '148,230p' specifyweb/specify/migration_utils/migration_helpers/helper_0040_components.py
echo
echo "== test context =="
sed -n '170,215p' specifyweb/specify/migration_utils/tests/test_helper_0040_components.py
echo
echo "== references to ishidden in this helper =="
rg -n "ishidden|reverse_hide_component_fields|hide_component_fields" specifyweb/specify/migration_utils/migration_helpers/helper_0040_components.py specifyweb/specify/migration_utils/tests/test_helper_0040_components.pyRepository: specify/specify7
Length of output: 4480
test_reverse_hide_component_fields should assert False
reverse_hide_component_fields updates matching items to ishidden=False, so this test’s final assertion is inverted.
🤖 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_0040_components.py`
around lines 195 - 202, The test_reverse_hide_component_fields assertion is
inverted: after calling reverse_hide_component_fields(apps), the hidden_field
should be verified as unhidden. Update the final assertion in
test_reverse_hide_component_fields so it checks the refreshed
self.hidden_field.ishidden is False, matching the behavior of
helper_0040_components.reverse_hide_component_fields.
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
Bug Fixes
Tests