Backend test - #104
Backend test#104jayam04 wants to merge 73 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughTest coverage is expanded for custom pylint checkers and linter functionality. The first file adds comprehensive unit tests for six custom checkers (HangingIndentChecker, DocstringParameterChecker, InequalityWithNoneChecker, DisallowHandlerWithoutSchemaChecker, DisallowedImportsChecker, PreventStringConcatenationChecker) and a RegisterFunctionTests suite. The second file adds three new test scenarios for linter selection, file exclusion, and duplicate detection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
|
@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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/linters/pylint_extensions_test.py`:
- Around line 5252-5270: The two tests testing "from typing import Text" are
redundant: either remove test_visit_importfrom_adds_message_for_text_import or
change it to cover a distinct case (e.g., multi-name import or aliased import)
so it does not duplicate
test_importing_text_from_typing_in_single_line_raises_error; update or rename
test_visit_importfrom_adds_message_for_text_import (and its assertion) to assert
a different behavior such as handling "from typing import Text as T" or "from
typing import Text, Other" using visit_importfrom, or delete it if redundant,
keeping test_visit_importfrom_returns_early_for_non_typing_import intact.
In `@scripts/linters/run_lint_checks_test.py`:
- Line 28: The import line "from typing import Dict, List, Optional" creates
inconsistent usage with newer annotations used later (e.g., list[tuple[...]]) —
change the type imports and annotations to use modern built-in generics: replace
Dict and List usages with dict and list in annotations (and optionally replace
Optional[T] with T | None or keep Optional if you prefer), and remove Dict/List
from the import statement so all type hints (including any annotations
referenced in functions or variables) consistently use the built-in dict/list
forms; update any annotations referenced in functions like any test helpers or
fixtures that currently use List[...] or Dict[...] to the built-in equivalents.
🪄 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: 02cc8ed3-b463-4fbc-8f05-adb45a47d075
📒 Files selected for processing (2)
scripts/linters/pylint_extensions_test.pyscripts/linters/run_lint_checks_test.py
| def test_visit_importfrom_adds_message_for_text_import(self) -> None: | ||
|
|
||
| import_node = astroid.extract_node('from typing import Text') | ||
|
|
||
| with self.checker_test_object.assertAddsMessages( | ||
| testutils.MessageTest( | ||
| msg_id='disallowed-text-import', | ||
| node=import_node, | ||
| ), | ||
| ignore_position=True, | ||
| ): | ||
| self.checker_test_object.checker.visit_importfrom(import_node) | ||
|
|
||
| def test_visit_importfrom_returns_early_for_non_typing_import(self) -> None: | ||
|
|
||
| import_node = astroid.extract_node('from os import path') | ||
|
|
||
| with self.checker_test_object.assertNoMessages(): | ||
| self.checker_test_object.checker.visit_importfrom(import_node) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor redundancy with existing test.
The test_visit_importfrom_adds_message_for_text_import test (lines 5252-5263) is very similar to the existing test_importing_text_from_typing_in_single_line_raises_error test (lines 5212-5223). Both test from typing import Text. Consider consolidating or differentiating the test names to clarify the unique purpose of each.
The test_visit_importfrom_returns_early_for_non_typing_import is a good negative test case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/linters/pylint_extensions_test.py` around lines 5252 - 5270, The two
tests testing "from typing import Text" are redundant: either remove
test_visit_importfrom_adds_message_for_text_import or change it to cover a
distinct case (e.g., multi-name import or aliased import) so it does not
duplicate test_importing_text_from_typing_in_single_line_raises_error; update or
rename test_visit_importfrom_adds_message_for_text_import (and its assertion) to
assert a different behavior such as handling "from typing import Text as T" or
"from typing import Text, Other" using visit_importfrom, or delete it if
redundant, keeping test_visit_importfrom_returns_early_for_non_typing_import
intact.
| from core.tests import test_utils | ||
|
|
||
| from typing import List, Optional | ||
| from typing import Dict, List, Optional |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using modern built-in generic types for consistency.
"The generic aliases in the typing module are considered deprecated and type checkers may warn if they are used." While the existing codebase uses List from typing throughout this file, the new code at line 388 uses the modern list[tuple[...]] syntax. This creates an inconsistency.
Given that the rest of the file uses the older style, this is acceptable for maintaining consistency with existing patterns, but consider migrating the entire file to modern syntax (dict, list) in a future cleanup.
🧰 Tools
🪛 Ruff (0.15.11)
[warning] 28-28: typing.Dict is deprecated, use dict instead
(UP035)
[warning] 28-28: typing.List is deprecated, use list instead
(UP035)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/linters/run_lint_checks_test.py` at line 28, The import line "from
typing import Dict, List, Optional" creates inconsistent usage with newer
annotations used later (e.g., list[tuple[...]]) — change the type imports and
annotations to use modern built-in generics: replace Dict and List usages with
dict and list in annotations (and optionally replace Optional[T] with T | None
or keep Optional if you prefer), and remove Dict/List from the import statement
so all type hints (including any annotations referenced in functions or
variables) consistently use the built-in dict/list forms; update any annotations
referenced in functions like any test helpers or fixtures that currently use
List[...] or Dict[...] to the built-in equivalents.
|
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
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