Increased branch coverage for question_services, rights_manager, and … - #105
Increased branch coverage for question_services, rights_manager, and …#105jayam04 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughRefactors control flow in domain services from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/domain/question_services_test.py`:
- Around line 733-751: The test
test_apply_change_list_with_non_update_property_cmd only asserts question.id
after apply_change_list so mutations to other fields would go unnoticed; before
calling question_services.apply_change_list capture the full pre-state of the
question (e.g., deep copy or serialize the Question object returned by fetching
via self.question_id), then call
question_services.apply_change_list(self.question_id, change_list) and assert
the returned/loaded Question equals the captured pre-state (compare the whole
object or all relevant fields), referencing QuestionChange and
question_domain.CMD_MIGRATE_STATE_SCHEMA_TO_LATEST_VERSION to locate the change
creation and apply_change_list to locate where to compare.
In `@core/domain/rights_manager_test.py`:
- Around line 221-232: The test
test_check_can_modify_core_activity_roles_owned_action_but_not_owner relies on
self.user_a's default role permissions; instead create a dedicated test user
object or mock (e.g., local_user = UserActionsMock(...) or a new user_id and
user object) that explicitly has ACTION_MODIFY_CORE_ROLES_FOR_OWNED_ACTIVITY but
is not the owner of EXP_ID, then pass that explicit non-owner user into
rights_manager.check_can_modify_core_activity_roles (replace self.user_a usage)
so the test no longer depends on global/default role mappings; ensure the
created user_id differs from the exploration owner (self.user_id_b) and that the
mock/user exposes the same interface used by
check_can_modify_core_activity_roles.
In `@core/domain/rights_manager.py`:
- Around line 167-168: The else after the return is unnecessary; in the
function/method in rights_manager.py where you check activity_type and return on
the if branch, remove the else block and unindent the assertion so that assert
activity_type == constants.ACTIVITY_TYPE_COLLECTION follows directly after the
return-path, eliminating extra nesting (i.e., delete the "else:" and place the
assert at the same level as the preceding if).
- Around line 280-282: Replace the fallback assert checks for activity_type with
explicit branches and an error raise: in the block that currently does "else:
assert activity_type == constants.ACTIVITY_TYPE_COLLECTION;
_update_collection_summary(activity_rights)" change the control flow to check
"elif activity_type == constants.ACTIVITY_TYPE_COLLECTION:
_update_collection_summary(activity_rights)" and add a final "else: raise
ValueError(f'Unexpected activity_type: {activity_type}')" (do the same change
for the other similar assert usage). This ensures _update_collection_summary and
related code only run for the expected discriminator and throws a clear
exception when activity_type is invalid.
In `@core/domain/skill_domain_test.py`:
- Around line 378-395: In the two tests
test_update_explanation_with_none_existing_explanation and
test_update_explanation_with_different_content_id, after asserting the
explanation assignment, call self.skill.validate() to ensure post-update object
invariants are checked; locate the assertions that compare
self.skill.skill_contents.explanation to new_explanation in those test methods
and insert self.skill.validate() immediately after each assertEqual.
- Around line 870-880: The test test_convert_skill_contents_v4_dict_to_v5_dict
is too weak because it only asserts top-level keys that may already exist;
replace it with a deterministic assertion using a crafted v4-shaped fixture:
build a skill_contents_dict matching the v4 schema (e.g. containing
fields/structure that should be transformed by
Skill._convert_skill_contents_v4_dict_to_v5_dict), call
Skill._convert_skill_contents_v4_dict_to_v5_dict(skill_contents_dict), and
assert the converted dict equals the expected v5-shaped dict (or at minimum
assert specific transformed sub-structures and moved/renamed keys rather than
just top-level presence) to catch regressions in conversion logic.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 75134dd0-d1b3-48e8-969c-51e4b2e6e694
📒 Files selected for processing (5)
core/domain/question_services.pycore/domain/question_services_test.pycore/domain/rights_manager.pycore/domain/rights_manager_test.pycore/domain/skill_domain_test.py
| def test_apply_change_list_with_non_update_property_cmd(self) -> None: | ||
| # A change with cmd != CMD_UPDATE_QUESTION_PROPERTY should be | ||
| # silently skipped, covering the False branch of that `if`. | ||
| change_list = [ | ||
| question_domain.QuestionChange( | ||
| { | ||
| 'cmd': ( | ||
| question_domain.CMD_MIGRATE_STATE_SCHEMA_TO_LATEST_VERSION | ||
| ), | ||
| 'from_version': 44, | ||
| 'to_version': 45, | ||
| } | ||
| ) | ||
| ] | ||
| question = question_services.apply_change_list( | ||
| self.question_id, change_list | ||
| ) | ||
| self.assertEqual(question.id, self.question_id) | ||
|
|
There was a problem hiding this comment.
Assertion is too weak for “unchanged question” behavior.
At Line 750, checking only id will pass even if other fields were mutated. Capture pre-state and assert full equality after applying the change list.
🔍 Suggested assertion upgrade
def test_apply_change_list_with_non_update_property_cmd(self) -> None:
+ original_question = question_services.get_question_by_id(self.question_id)
+ original_question_dict = original_question.to_dict()
# A change with cmd != CMD_UPDATE_QUESTION_PROPERTY should be
# silently skipped, covering the False branch of that `if`.
change_list = [
question_domain.QuestionChange(
{
'cmd': (
question_domain.CMD_MIGRATE_STATE_SCHEMA_TO_LATEST_VERSION
),
'from_version': 44,
'to_version': 45,
}
)
]
question = question_services.apply_change_list(
self.question_id, change_list
)
- self.assertEqual(question.id, self.question_id)
+ self.assertEqual(question.to_dict(), original_question_dict)📝 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.
| def test_apply_change_list_with_non_update_property_cmd(self) -> None: | |
| # A change with cmd != CMD_UPDATE_QUESTION_PROPERTY should be | |
| # silently skipped, covering the False branch of that `if`. | |
| change_list = [ | |
| question_domain.QuestionChange( | |
| { | |
| 'cmd': ( | |
| question_domain.CMD_MIGRATE_STATE_SCHEMA_TO_LATEST_VERSION | |
| ), | |
| 'from_version': 44, | |
| 'to_version': 45, | |
| } | |
| ) | |
| ] | |
| question = question_services.apply_change_list( | |
| self.question_id, change_list | |
| ) | |
| self.assertEqual(question.id, self.question_id) | |
| def test_apply_change_list_with_non_update_property_cmd(self) -> None: | |
| original_question = question_services.get_question_by_id(self.question_id) | |
| original_question_dict = original_question.to_dict() | |
| # A change with cmd != CMD_UPDATE_QUESTION_PROPERTY should be | |
| # silently skipped, covering the False branch of that `if`. | |
| change_list = [ | |
| question_domain.QuestionChange( | |
| { | |
| 'cmd': ( | |
| question_domain.CMD_MIGRATE_STATE_SCHEMA_TO_LATEST_VERSION | |
| ), | |
| 'from_version': 44, | |
| 'to_version': 45, | |
| } | |
| ) | |
| ] | |
| question = question_services.apply_change_list( | |
| self.question_id, change_list | |
| ) | |
| self.assertEqual(question.to_dict(), original_question_dict) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/domain/question_services_test.py` around lines 733 - 751, The test
test_apply_change_list_with_non_update_property_cmd only asserts question.id
after apply_change_list so mutations to other fields would go unnoticed; before
calling question_services.apply_change_list capture the full pre-state of the
question (e.g., deep copy or serialize the Question object returned by fetching
via self.question_id), then call
question_services.apply_change_list(self.question_id, change_list) and assert
the returned/loaded Question equals the captured pre-state (compare the whole
object or all relevant fields), referencing QuestionChange and
question_domain.CMD_MIGRATE_STATE_SCHEMA_TO_LATEST_VERSION to locate the change
creation and apply_change_list to locate where to compare.
| def test_check_can_modify_core_activity_roles_owned_action_but_not_owner( | ||
| self, | ||
| ) -> None: | ||
| # user_a has ACTION_MODIFY_CORE_ROLES_FOR_OWNED_ACTIVITY but is not | ||
| # the owner of exp_id, so the function should return False. | ||
| self.save_new_valid_exploration(self.EXP_ID, self.user_id_b) | ||
| exp_rights = rights_manager.get_exploration_rights(self.EXP_ID) | ||
| self.assertFalse( | ||
| rights_manager.check_can_modify_core_activity_roles( | ||
| self.user_a, exp_rights | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make this non-owner branch test independent of default role permissions.
At Line 229, this uses self.user_a, which ties branch intent to current default action mappings. Prefer an explicit mock/user object with ACTION_MODIFY_CORE_ROLES_FOR_OWNED_ACTIVITY and a non-owner user_id so this remains stable if role policies change.
✅ Deterministic test setup
def test_check_can_modify_core_activity_roles_owned_action_but_not_owner(
self,
) -> None:
- # user_a has ACTION_MODIFY_CORE_ROLES_FOR_OWNED_ACTIVITY but is not
- # the owner of exp_id, so the function should return False.
self.save_new_valid_exploration(self.EXP_ID, self.user_id_b)
exp_rights = rights_manager.get_exploration_rights(self.EXP_ID)
+ mock_user = unittest.mock.MagicMock(spec=user_domain.UserActionsInfo)
+ mock_user.user_id = self.user_id_a
+ mock_user.actions = [
+ role_services.ACTION_MODIFY_CORE_ROLES_FOR_OWNED_ACTIVITY
+ ]
self.assertFalse(
rights_manager.check_can_modify_core_activity_roles(
- self.user_a, exp_rights
+ mock_user, exp_rights
)
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/domain/rights_manager_test.py` around lines 221 - 232, The test
test_check_can_modify_core_activity_roles_owned_action_but_not_owner relies on
self.user_a's default role permissions; instead create a dedicated test user
object or mock (e.g., local_user = UserActionsMock(...) or a new user_id and
user object) that explicitly has ACTION_MODIFY_CORE_ROLES_FOR_OWNED_ACTIVITY but
is not the owner of EXP_ID, then pass that explicit non-owner user into
rights_manager.check_can_modify_core_activity_roles (replace self.user_a usage)
so the test no longer depends on global/default role mappings; ensure the
created user_id differs from the exploration owner (self.user_id_b) and that the
mock/user exposes the same interface used by
check_can_modify_core_activity_roles.
| else: | ||
| assert activity_type == constants.ACTIVITY_TYPE_COLLECTION |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the file
find . -name "rights_manager.py" -type f | head -5Repository: jayam04/oppia
Length of output: 89
🏁 Script executed:
# Read the specific section around line 167
sed -n '155,185p' core/domain/rights_manager.py | cat -nRepository: jayam04/oppia
Length of output: 1468
🏁 Script executed:
# Also check the other mentioned line ranges
echo "=== Lines 275-290 ===" && sed -n '275,290p' core/domain/rights_manager.py | cat -n
echo "=== Lines 418-430 ===" && sed -n '418,430p' core/domain/rights_manager.py | cat -n
echo "=== Lines 1156-1170 ===" && sed -n '1156,1170p' core/domain/rights_manager.py | cat -n
echo "=== Lines 1384-1400 ===" && sed -n '1384,1400p' core/domain/rights_manager.py | cat -nRepository: jayam04/oppia
Length of output: 2875
🏁 Script executed:
# Check the function signature and broader context around _save_activity_rights
sed -n '150,200p' core/domain/rights_manager.py | cat -nRepository: jayam04/oppia
Length of output: 2351
Remove unnecessary else after return at line 167.
The if branch returns at line 166, making the else block and its indentation unnecessary. This violates PEP 8's principle of avoiding unnecessary nesting and triggers the RET505 lint warning.
Proposed fix
if activity_type == constants.ACTIVITY_TYPE_EXPLORATION:
assert isinstance(activity_rights, exp_rights_domain.ExplorationRights)
_save_exploration_rights(
committer_id, activity_rights, commit_message, commit_cmds
)
return
- else:
- assert activity_type == constants.ACTIVITY_TYPE_COLLECTION
- model = collection_models.CollectionRightsModel.get(
- activity_rights.id, strict=True
- )
+ assert activity_type == constants.ACTIVITY_TYPE_COLLECTION
+ model = collection_models.CollectionRightsModel.get(
+ activity_rights.id, strict=True
+ )Per PEP 8: avoid unnecessary nesting and indentation after control flow statements that terminate execution.
🧰 Tools
🪛 Ruff (0.15.11)
[warning] 167-167: Unnecessary else after return statement
Remove unnecessary else
(RET505)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/domain/rights_manager.py` around lines 167 - 168, The else after the
return is unnecessary; in the function/method in rights_manager.py where you
check activity_type and return on the if branch, remove the else block and
unindent the assertion so that assert activity_type ==
constants.ACTIVITY_TYPE_COLLECTION follows directly after the return-path,
eliminating extra nesting (i.e., delete the "else:" and place the assert at the
same level as the preceding if).
| else: | ||
| assert activity_type == constants.ACTIVITY_TYPE_COLLECTION | ||
| _update_collection_summary(activity_rights) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Check whether optimized Python mode might be used anywhere.
rg -n --iglob '*.yml' --iglob '*.yaml' --iglob '*.sh' --iglob '*.py' \
'PYTHONOPTIMIZE|python\s+-O|python3\s+-O'
# 2) Enumerate callsites of the affected helpers.
rg -nP '\b(_update_activity_summary|_change_activity_status)\s*\('Repository: jayam04/oppia
Length of output: 697
🏁 Script executed:
sed -n '275,290p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 633
🏁 Script executed:
sed -n '1385,1400p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 769
🏁 Script executed:
sed -n '260,285p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 987
🏁 Script executed:
sed -n '1358,1375p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 620
🏁 Script executed:
# Check all callsites of _update_activity_summary
sed -n '1195,1210p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 457
🏁 Script executed:
# Check callsite around line 1300
sed -n '1295,1310p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 538
🏁 Script executed:
# Check callsite around line 1357
sed -n '1352,1365p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 406
🏁 Script executed:
# Check callsites of _change_activity_status
sed -n '1437,1450p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 463
🏁 Script executed:
# Check the other _change_activity_status callsite
sed -n '1475,1488p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 544
🏁 Script executed:
# Find the publish_activity and unpublish_activity function signatures
rg -n 'def (publish_activity|unpublish_activity)' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 39
🏁 Script executed:
# Look at the beginning of publish_activity to see if activity_type is validated
sed -n '1410,1450p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 1461
🏁 Script executed:
# Look at unpublish_activity
sed -n '1460,1500p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 1428
🏁 Script executed:
# Find _get_activity_rights to see if it validates activity_type
rg -n 'def _get_activity_rights' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 227
🏁 Script executed:
# Read _get_activity_rights implementation
sed -n '220,260p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 1271
🏁 Script executed:
# Look for any other functions that accept activity_type and might have it unvalidated
rg -n 'activity_type:' core/domain/rights_manager.py | head -30Repository: jayam04/oppia
Length of output: 1613
🏁 Script executed:
# Read _get_activity_rights implementation
sed -n '656,700p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 1795
🏁 Script executed:
# Check if any validation happens in calling functions - look at who calls _publish_activity/_unpublish_activity
rg -n '(publish_activity|unpublish_activity|_assign_role)\(' core/domain/rights_manager.py | head -15Repository: jayam04/oppia
Length of output: 595
🏁 Script executed:
# Check the public API entry points like assign_role_for_exploration
sed -n '1500,1550p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 1580
🏁 Script executed:
# Check _deassign_role implementation
sed -n '1203,1280p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 3096
🏁 Script executed:
# Check _release_ownership_of_activity implementation
sed -n '1305,1360p' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 1923
🏁 Script executed:
# Verify the call sequence - check if _get_activity_rights is always called before _update_activity_summary
grep -B 10 '_update_activity_summary' core/domain/rights_manager.py | grep -E '(_get_activity_rights|_update_activity_summary)' | head -20Repository: jayam04/oppia
Length of output: 331
🏁 Script executed:
# Double-check the exception handling in _get_activity_rights to confirm it raises for invalid types
sed -n '656,690p' core/domain/rights_manager.py | grep -A 20 'else:'Repository: jayam04/oppia
Length of output: 231
🏁 Script executed:
# Verify if _update_activity_summary and _change_activity_status can be called directly
# Check if they are marked as private (leading underscore) and search for direct calls without _get_activity_rights
rg -n '_update_activity_summary|_change_activity_status' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 457
🏁 Script executed:
# Check if there are any public API functions that might call these directly
rg -n '^def [a-z].*activity_type' core/domain/rights_manager.pyRepository: jayam04/oppia
Length of output: 39
Replace assert discriminator checks with explicit exception handling.
At lines 281 and 1390, the code uses assert statements as fallback checks for invalid activity_type values. While _get_activity_rights() does validate activity_type upstream, defensive programming practices recommend replacing these asserts with explicit exceptions. This makes the code more robust and documents the intent clearly, especially since assert statements could theoretically be disabled (even though PYTHONOPTIMIZE is not currently used in the repository).
Consider the suggested fix to replace both else: assert activity_type == ... blocks with elif activity_type == ...: ... else: raise Exception(...) patterns for clarity and safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/domain/rights_manager.py` around lines 280 - 282, Replace the fallback
assert checks for activity_type with explicit branches and an error raise: in
the block that currently does "else: assert activity_type ==
constants.ACTIVITY_TYPE_COLLECTION; _update_collection_summary(activity_rights)"
change the control flow to check "elif activity_type ==
constants.ACTIVITY_TYPE_COLLECTION: _update_collection_summary(activity_rights)"
and add a final "else: raise ValueError(f'Unexpected activity_type:
{activity_type}')" (do the same change for the other similar assert usage). This
ensures _update_collection_summary and related code only run for the expected
discriminator and throws a clear exception when activity_type is invalid.
| def test_update_explanation_with_none_existing_explanation(self) -> None: | ||
| # Here we use MyPy ignore because the explanation field is typed as | ||
| # SubtitledHtml (not Optional), but update_explanation handles a falsy | ||
| # explanation at runtime to cover that branch. | ||
| self.skill.skill_contents.explanation = None # type: ignore[assignment] | ||
| new_explanation = state_domain.SubtitledHtml( | ||
| '4', '<p>New Explanation</p>' | ||
| ) | ||
| self.skill.update_explanation(new_explanation) | ||
| self.assertEqual(self.skill.skill_contents.explanation, new_explanation) | ||
|
|
||
| def test_update_explanation_with_different_content_id(self) -> None: | ||
| new_explanation = state_domain.SubtitledHtml( | ||
| '4', '<p>New Explanation</p>' | ||
| ) | ||
| self.skill.update_explanation(new_explanation) | ||
| self.assertEqual(self.skill.skill_contents.explanation, new_explanation) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen explanation-branch tests with post-update validation.
These tests cover branches, but adding self.skill.validate() after Line 386 and Line 393 would verify object invariants, not just assignment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/domain/skill_domain_test.py` around lines 378 - 395, In the two tests
test_update_explanation_with_none_existing_explanation and
test_update_explanation_with_different_content_id, after asserting the
explanation assignment, call self.skill.validate() to ensure post-update object
invariants are checked; locate the assertions that compare
self.skill.skill_contents.explanation to new_explanation in those test methods
and insert self.skill.validate() immediately after each assertEqual.
| def test_convert_skill_contents_v4_dict_to_v5_dict(self) -> None: | ||
| skill_contents_dict: skill_domain.SkillContentsDict = ( | ||
| self.skill.skill_contents.to_dict() | ||
| ) | ||
| result = skill_domain.Skill._convert_skill_contents_v4_dict_to_v5_dict( # pylint: disable=protected-access | ||
| skill_contents_dict | ||
| ) | ||
| self.assertIn('explanation', result) | ||
| self.assertIn('recorded_voiceovers', result) | ||
| self.assertIn('written_translations', result) | ||
|
|
There was a problem hiding this comment.
Conversion test is currently too weak to catch migration regressions.
At Line 877–879, asserting top-level key presence does not prove v4→v5 conversion behavior, since those keys can already exist before conversion. Please assert a concrete transformed output from a real v4-shaped fixture.
✅ Suggested test hardening
def test_convert_skill_contents_v4_dict_to_v5_dict(self) -> None:
- skill_contents_dict: skill_domain.SkillContentsDict = (
- self.skill.skill_contents.to_dict()
- )
+ # Use a genuine v4-shaped payload fixture here, then assert exact
+ # transformed values expected in v5.
+ skill_contents_dict: skill_domain.SkillContentsDict = (
+ self.skill.skill_contents.to_dict()
+ )
result = skill_domain.Skill._convert_skill_contents_v4_dict_to_v5_dict( # pylint: disable=protected-access
skill_contents_dict
)
- self.assertIn('explanation', result)
- self.assertIn('recorded_voiceovers', result)
- self.assertIn('written_translations', result)
+ self.assertEqual(result, expected_v5_skill_contents_dict)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/domain/skill_domain_test.py` around lines 870 - 880, The test
test_convert_skill_contents_v4_dict_to_v5_dict is too weak because it only
asserts top-level keys that may already exist; replace it with a deterministic
assertion using a crafted v4-shaped fixture: build a skill_contents_dict
matching the v4 schema (e.g. containing fields/structure that should be
transformed by Skill._convert_skill_contents_v4_dict_to_v5_dict), call
Skill._convert_skill_contents_v4_dict_to_v5_dict(skill_contents_dict), and
assert the converted dict equals the expected v5-shaped dict (or at minimum
assert specific transformed sub-structures and moved/renamed keys rather than
just top-level presence) to catch regressions in conversion logic.
|
Hi @DubeySandeep, @oppia/web-tech-leads I cannot decide what to do with this PR, please assign reviewers manually thanks! |
2 similar comments
|
Hi @DubeySandeep, @oppia/web-tech-leads I cannot decide what to do with this PR, please assign reviewers manually thanks! |
|
Hi @DubeySandeep, @oppia/web-tech-leads I cannot decide what to do with this PR, please assign reviewers manually thanks! |
2 participants
…skill_domain
Overview
the cause of the bug was, and which PR introduced it]
Essential Checklist
Please follow the instructions for making a code change.
Testing doc (for PRs with Beam jobs that modify production server data)
Proof that changes are correct
Proof of changes on desktop with slow/throttled network
Proof of changes on mobile phone
Proof of changes in Arabic language
PR Pointers
Summary by CodeRabbit
Tests
Refactor