Implement idempotency test for run_key_migration_functions#8050
Open
acwhite211 wants to merge 6 commits into
Open
Implement idempotency test for run_key_migration_functions#8050acwhite211 wants to merge 6 commits into
run_key_migration_functions#8050acwhite211 wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR adds a Django test module that simulates Specify 7 usage, runs the ChangesMigration Idempotency Verification
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 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 |
Triggered by 6c68f2b on branch refs/heads/issue-8042
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
specifyweb/specify/tests/test_run_key_migration_functions.py (3)
322-325: ⚡ Quick winFirst-run assertion may not catch partial migration failures.
The assertion only verifies that some tracked model saw an increase, not which models or by how much. If the migration incorrectly skips creating certain expected record types (e.g., only creates UniquenessRule but fails to create Picklist records), the test would still pass.
Consider asserting on specific expected models or minimum thresholds to better verify the migration's completeness.
🔍 Example: Assert on expected models
first_run_diff = count_diff(before_first_run, after_first_run) -self.assertTrue( - any(change > 0 for change in first_run_diff.values()), - f"Expected first run to create or backfill records. Diff: {first_run_diff}", -) +# Verify that the first run created records (exact models depend on migration logic) +self.assertGreater( + len(first_run_diff), + 0, + f"Expected first run to create or backfill records. Diff: {first_run_diff}", +) +# Example: Assert specific models if known +# self.assertIn("Picklist", first_run_diff) +# self.assertGreater(first_run_diff["Picklist"], 0)🤖 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/tests/test_run_key_migration_functions.py` around lines 322 - 325, The test's first-run assertion (using first_run_diff) only checks that some tracked model increased and can miss partial failures; update the assertion to check specific expected models and/or minimum counts (e.g., assert first_run_diff["UniquenessRule"] > 0 and first_run_diff["Picklist"] > 0 or similar) so the test verifies creation/backfill of each required record type; modify the test that uses first_run_diff (in test_run_key_migration_functions.py) to assert per-model increases or thresholds rather than any(change > 0).
20-40: 💤 Low valueConsider standardizing TRACKED_MODELS key casing.
The keys mix different conventions:
"Collectionobjecttype"(all lowercase after first letter),"UniquenessRule"(PascalCase), and"LibraryRole"(PascalCase). Standardizing to either all PascalCase or all lowercase would improve readability.🤖 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/tests/test_run_key_migration_functions.py` around lines 20 - 40, TRACKED_MODELS contains mixed key casing; make the keys consistent by replacing the hard-coded mixed-case strings with a single consistent convention (for example, use each model class's __name__ or normalized PascalCase) so keys match their model identifiers; update the TRACKED_MODELS declaration (the dict with keys like "Collectionobjecttype", "UniquenessRule", "LibraryRole", etc.) to generate or use uniform keys (e.g., model.__name__ or a normalized casing function) for all entries so lookups are predictable and readable.
145-145: 💤 Low valueSuffix format assumption is fragile.
The schema container name construction assumes
suffixcontains hyphens. While current test calls use hyphenated suffixes, this creates a hidden dependency between the test method and helper.Consider using a more robust transformation or documenting the suffix format requirement.
🤖 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/tests/test_run_key_migration_functions.py` at line 145, The test currently builds the schema container name with name=f"test_schema_container_{suffix.replace('-', '_')}", which assumes suffix contains hyphens; update the construction to robustly normalize suffix (e.g., replace any non-alphanumeric characters with underscores) so tests don't depend on hyphen-only input, by changing the name formatting where suffix is used (the expression creating name in the test helper or test function) to use a general sanitization/normalization routine (or call a small helper sanitize_suffix) and/or add a brief comment documenting the expected allowed characters for suffix.
🤖 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.
Nitpick comments:
In `@specifyweb/specify/tests/test_run_key_migration_functions.py`:
- Around line 322-325: The test's first-run assertion (using first_run_diff)
only checks that some tracked model increased and can miss partial failures;
update the assertion to check specific expected models and/or minimum counts
(e.g., assert first_run_diff["UniquenessRule"] > 0 and
first_run_diff["Picklist"] > 0 or similar) so the test verifies
creation/backfill of each required record type; modify the test that uses
first_run_diff (in test_run_key_migration_functions.py) to assert per-model
increases or thresholds rather than any(change > 0).
- Around line 20-40: TRACKED_MODELS contains mixed key casing; make the keys
consistent by replacing the hard-coded mixed-case strings with a single
consistent convention (for example, use each model class's __name__ or
normalized PascalCase) so keys match their model identifiers; update the
TRACKED_MODELS declaration (the dict with keys like "Collectionobjecttype",
"UniquenessRule", "LibraryRole", etc.) to generate or use uniform keys (e.g.,
model.__name__ or a normalized casing function) for all entries so lookups are
predictable and readable.
- Line 145: The test currently builds the schema container name with
name=f"test_schema_container_{suffix.replace('-', '_')}", which assumes suffix
contains hyphens; update the construction to robustly normalize suffix (e.g.,
replace any non-alphanumeric characters with underscores) so tests don't depend
on hyphen-only input, by changing the name formatting where suffix is used (the
expression creating name in the test helper or test function) to use a general
sanitization/normalization routine (or call a small helper sanitize_suffix)
and/or add a brief comment documenting the expected allowed characters for
suffix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 100c0c3d-be1b-4077-855d-3f7b1fb63ce9
📒 Files selected for processing (1)
specifyweb/specify/tests/test_run_key_migration_functions.py
Member
Author
|
This one seems good to merge |
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 #8042
Adds a Django regression test for the
run_key_migration_functionsmanagement command to verify that it can be run repeatedly without creating duplicate migration-backed records.The test snapshots relevant database record counts before the command runs, after the first run, and after a second run. The first run is expected to create or backfill records, while the second run must leave tracked record counts unchanged.
There is tracking on counts for models touched by the command, including collection object types, picklists, schema config records, permissions records, app resource dirs, uniqueness rules, and tectonic unit records. I also went ahead and added user-created picklist and permission records before the first run and between runs to simulate normal Specify 7 usage. We could try doing more here to simulate other Specify 7 usage.
Checklist
self-explanatory (or properly documented)
Testing instructions
Summary by CodeRabbit