Enhance test quality #146
Enhance test quality #146
Conversation
WalkthroughAdd type annotations across many tests and fixtures, introduce shared pytest conftest modules for test import resolution, and extend static-analysis tooling to include tests and the lsc_agent_eval subtree (mypy/pylint/pyright). Minor typing edits added to two script modules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/run_multi_provider_eval.py (1)
784-834: Fix the type definition instead of using# type: ignorecomments.The coding guidelines specify not to use
# type: ignorecomments. The issue is thatscore_stats["confidence_interval"]is initially typed as a dict but later assignedNone. Instead of suppressing the type error, define the type to properly acceptNone.Proposed fix
if all_scores: - score_stats: dict[str, Any] = { + score_stats: dict[str, Any] = { "mean": float(np.mean(all_scores)), "median": float(np.median(all_scores)), "std": float(np.std(all_scores)), "min": float(np.min(all_scores)), "max": float(np.max(all_scores)), "count": len(all_scores), + "confidence_interval": None, # Will be populated below if applicable } # Calculate confidence interval for mean score (95% confidence) if len(all_scores) > 1: try: from scipy import stats as scipy_stats confidence_level = 0.95 degrees_freedom = len(all_scores) - 1 sample_std = float(np.std(all_scores, ddof=1)) std_error = sample_std / np.sqrt(len(all_scores)) # Using t-distribution critical value t_critical = scipy_stats.t.ppf( (1 + confidence_level) / 2, degrees_freedom ) margin_of_error = t_critical * std_error score_stats["confidence_interval"] = { "low": float( max(0.0, score_stats["mean"] - margin_of_error) ), # Clamp to 0 "high": float( min(1.0, score_stats["mean"] + margin_of_error) ), # Clamp to 1 "mean": float(score_stats["mean"]), "confidence_level": confidence_level * 100, } except ImportError: logger.warning( "scipy not available, skipping confidence interval calculation" ) - score_stats["confidence_interval"] = None # type: ignore[assignment] - else: - # Single score - no confidence interval - score_stats["confidence_interval"] = None # type: ignore[assignment] + # confidence_interval already initialized to None + # else: Single score - confidence_interval already initialized to NoneAs per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead".
🤖 Fix all issues with AI agents
In `@tests/script/test_compare_evaluations.py`:
- Around line 147-149: The tests are calling the protected method
_compare_score_distributions (and several other protected methods) via a pylint
disable; instead add or use a public API/wrapper in the module (e.g., a new
public helper function compare_score_distributions or a test-only wrapper
function) that delegates to comparison_instance._compare_score_distributions,
update the tests to call that public helper instead of the protected method, and
remove the "# pylint: disable=protected-access" comments; ensure the helper is
exported from the module where Comparison/ComparisonInstance is defined so tests
can import it without accessing protected members.
In `@tests/script/test_run_multi_provider_eval.py`:
- Around line 244-246: The tests currently suppress linting (e.g.,
protected-access, too-few-public-methods, consider-using-with) around calls like
runner._load_yaml and the temp file helper in temp_config_files; remove those #
pylint: disable comments and fix the root causes by (1) using the public API
instead of calling the protected method runner._load_yaml (expose a public
runner.load_yaml or add a test-only public hook method), and (2) replacing the
temp_config_files usage with a proper context-manager based temp file helper (or
use pytest tmp_path/tmp_path_factory) so files are opened/closed safely; apply
the same changes wherever similar disables are present in this test file by
switching to public methods or adding minimal test hooks and context-managed
temp file usage.
- Line 7: Replace the unittest.patch import and context managers with the
pytest-mock fixture: remove "from unittest.mock import patch", update tests to
accept a "mocker: MockerFixture" parameter, and replace "with patch(...)" uses
by "mocker.patch(...)" (or "mocker.patch.object(...)") in each test. Remove all
"# pylint: disable=protected-access" comments and refactor tests that call
protected members (e.g., "_run_single_evaluation") to use public APIs or test
helpers instead (call the public method that triggers the evaluation flow or
expose a test fixture/wrapper that invokes the internal behavior). Ensure any
patch targets reference the same symbols used in the diff (function/class names)
so mocks still apply after converting to mocker.patch.
In `@tests/unit/core/api/test_streaming_parser.py`:
- Around line 227-231: The test test_parse_tool_call_invalid_structure uses a
pyright ignore; instead make the token variable type-safe by importing Any from
typing and declaring token: Any = "not a dict", remove the "# pyright: ignore"
comment, and keep the rest of the test calling _parse_tool_call(token) so the
invalid-structure behavior is exercised without disabling linting; locate the
assignment in test_parse_tool_call_invalid_structure and update the declaration
accordingly.
In `@tests/unit/core/metrics/conftest.py`:
- Line 70: The fixtures currently use pylint disable comments to suppress
redefined-outer-name (e.g., the sample_scope fixture and the sample_turn_data
fixture); rename the actual fixture function to have a leading underscore (e.g.,
_sample_turn_data) and change the decorator to
`@pytest.fixture`(name="sample_turn_data") so the fixture is still exposed with
the original name but no longer shadows an outer name; update any other similar
fixtures (the one referenced at lines ~82) the same way and remove the
corresponding "# pylint: disable=redefined-outer-name" comments.
- Around line 93-142: The fixtures mock_bleu_scorer, mock_rouge_scorer, and
mock_similarity_scorer currently declare their return type as MockerFixture but
actually return MagicMock instances; change their return annotations to Either
Any or unittest.mock.MagicMock (e.g., -> Any or -> MagicMock) and add the
corresponding import if needed, and then update any test signatures in
tests/unit/core/metrics/test_nlp.py that annotate injected parameters for these
fixtures to match the new return types so type checkers are accurate.
In `@tests/unit/core/metrics/test_nlp.py`:
- Around line 295-302: Refactor the
test_ragas_metric_failure_raises_metric_error signature to remove the pylint
disable by bundling metric_name and scorer_path into a single fixture (e.g., a
dataclass or namedtuple like MetricCase) that returns both values; update the
parametrization to produce MetricCase instances and change the test parameter
list to (self, nlp_metrics, sample_scope, metric_case) and use
metric_case.metric_name / metric_case.scorer_path inside the test, removing the
extra parameters and the "# pylint:
disable=too-many-arguments,too-many-positional-arguments" comment.
In `@tests/unit/core/output/test_final_coverage.py`:
- Line 137: Remove the pylint disable by addressing the underlying issue in
class TestSystemLoaderEdgeCases: either convert the class into a module-level
test function (rename the single test method to a top-level function and remove
the class wrapper) or add at least one more related test method covering another
edge case for the same behavior so the class legitimately contains multiple
public tests; then delete the "# pylint: disable=too-few-public-methods" comment
and run the test/lint suite to confirm the warning is gone.
- Line 100: The test currently calls private methods _calculate_stats and
_generate_csv_report directly; remove the protected-access suppression by either
(A) exercising the public API generate_reports() in the test and asserting the
resulting outputs/side-effects that cover the same logic, or (B) if independent
unit tests are required, make those methods public by renaming _calculate_stats
-> calculate_stats and _generate_csv_report -> generate_csv_report (and update
any call sites), then update tests to call the new public methods; ensure
assertions validate the same behavior originally checked by the private calls.
In `@tests/unit/core/output/test_generator.py`:
- Line 29: Replace direct calls to protected methods (e.g.,
OutputHandler._calculate_stats, _generate_csv_report, _generate_json_summary,
_generate_text_summary) in the tests with calls to the public
OutputHandler.generate_reports() API; remove all corresponding "# pylint:
disable=protected-access" comments and instead assert on the generated output
files and their contents (as done in test_generate_reports_creates_files,
test_generate_reports_with_empty_results, test_generate_reports_without_config)
to verify behavior. Ensure each affected test constructs the same handler
input/state, invokes handler.generate_reports(), then checks for expected
files/contents rather than inspecting internal helper methods.
- Line 50: The type annotation is incorrect: the fixture mock_system_config
returns a mock object (mocker.Mock()), not a MockerFixture; update the fixture
signature named mock_system_config in conftest and all test parameter
annotations (the mock_system_config parameters in test_generator.py) to use a
more accurate type such as unittest.mock.MagicMock or typing.Any (replace
MockerFixture with MagicMock/Any) so the declared return type matches the actual
mock object returned by mocker.Mock().
In `@tests/unit/core/system/test_validator.py`:
- Around line 58-62: The test currently calls the internal helper
validator._validate_evaluation_data with a pylint disable; remove the
protected-access suppression and test via the public API instead (e.g., call
validator.validate_evaluation or whichever public method exercises the same
validation path) or, if this helper is intended to be part of the test surface,
make it public (rename _validate_evaluation_data to validate_evaluation_data)
and update tests to call the public name; apply the same change for the other
occurrences listed so tests no longer use pylint: disable=protected-access.
🟡 Minor comments (20)
tests/unit/core/llm/test_custom.py-23-25 (1)
23-25: Avoid inline# pylint: disable=protected-accesscomments.While testing protected methods is sometimes necessary for thorough unit testing, the coding guidelines discourage inline disable comments. Consider either:
- Configuring pylint at project level to allow protected access in test files
- Testing through public interfaces if possible
Project-level configuration alternative
In
pyproject.toml:[tool.pylint.main] # Allow protected access in test files extension-pkg-allow-list = ["tests"]Or use a separate
.pylintrcfor tests.As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead".
tests/unit/core/llm/test_custom.py-10-11 (1)
10-11: Avoid# pylint: disablecomments per coding guidelines.The coding guidelines specify not to use
# pylint: disablecomments. For test classes,too-few-public-methodsis typically not a concern and can be configured at the project level inpyproject.tomlor.pylintrcto exclude test files.Suggested project-level configuration
In
pyproject.toml, you can exclude test files from this check:[tool.pylint."messages control"] disable = ["too-few-public-methods"] # Or configure min-public-methods for test directoriesAs per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead".
tests/unit/pipeline/evaluation/test_processor.py-42-48 (1)
42-48: Remove unusedmockerparameter or use it.The
mocker: MockerFixtureparameter is marked with# pylint: disable=unused-argumentbut the test doesn't use it. Either remove the parameter if it's not needed, or use it for mocking if intended.def test_process_conversation_skips_when_no_metrics( self, mock_config_loader: ConfigLoader, processor_components: ProcessorComponents, sample_conv_data: EvaluationData, - mocker: MockerFixture, # pylint: disable=unused-argument ) -> None:As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead".
tests/unit/pipeline/evaluation/test_processor.py-375-377 (1)
375-377: Consistent# pylint: disable=protected-accesspattern throughout file.This file has many inline pylint disable comments for protected method access. This is a recurring pattern across multiple test files in this PR. Consider addressing this at the project configuration level.
As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead".
Also applies to: 405-407, 424-426, 443-445, 464-466
tests/unit/core/api/test_client.py-251-253 (1)
251-253: Multiple# pylint: disable=protected-accesscomments violate coding guidelines.This file has numerous inline pylint disable comments for protected access. Consider configuring pylint at the project level to allow protected access in test files rather than using inline comments.
Centralized configuration approach
In
pyproject.toml, add configuration to ignore protected-access warnings in test files:[tool.pylint.main] ignore-patterns = ["test_.*\\.py"] # Or in a separate tests/.pylintrc: [MESSAGES CONTROL] disable=protected-accessAs per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead".
Also applies to: 266-268, 284-286
tests/script/conftest.py-128-137 (1)
128-137: Avoid# pylint: disable=redefined-outer-namecomment.The coding guidelines specify not to use pylint disable comments. The
redefined-outer-namewarning is common for pytest fixtures that depend on other fixtures. Consider renaming the parameter or configuring pylint at project level for test files.Alternative approaches
Option 1: Rename the parameter
`@pytest.fixture` -def runner( # pylint: disable=redefined-outer-name - temp_config_files: dict, +def runner( + config_files: dict, # Renamed to avoid shadowing ) -> MultiProviderEvaluationRunner:Option 2: Project-level configuration in
pyproject.toml:[tool.pylint."messages control"] disable = ["redefined-outer-name"]As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead".
tests/unit/runner/test_evaluation.py-261-265 (1)
261-265: Remove# pylint: disable=unused-argumentcomment.Same issue - use
_prefix convention for the unused parameter.🔧 Suggested fix
def test_run_evaluation_closes_pipeline_on_exception( self, mocker: MockerFixture, - capsys: pytest.CaptureFixture, # pylint: disable=unused-argument + _capsys: pytest.CaptureFixture, ) -> None:tests/unit/runner/test_evaluation.py-98-102 (1)
98-102: Remove# pylint: disable=unused-argumentcomment.Same issue as above - use
_prefix for intentionally unused parameters instead of suppressing the warning.🔧 Suggested fix
def test_run_evaluation_with_output_dir_override( self, mocker: MockerFixture, - capsys: pytest.CaptureFixture, # pylint: disable=unused-argument + _capsys: pytest.CaptureFixture, ) -> None:tests/unit/pipeline/evaluation/conftest.py-199-214 (1)
199-214: Remove# pylint: disable=redefined-outer-namecomments.The coding guidelines prohibit disabling lint warnings. For pytest fixtures, the
redefined-outer-namewarning is a known false positive when fixtures use other fixtures as parameters.Consider configuring pylint globally in
pyproject.tomlto ignore this pattern in test files, rather than inline suppressions:[tool.pylint."messages control"] disable = ["redefined-outer-name"]Or rename fixture parameters to avoid shadowing (e.g.,
metrics_evaluatorinstead ofmock_metrics_evaluator).As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
tests/unit/runner/test_evaluation.py-28-32 (1)
28-32: Remove# pylint: disable=unused-argumentcomment.The coding guidelines prohibit disabling lint warnings with
# pylint: disablecomments. Ifcapsysis needed for side effects (capturing output during test execution), consider either:
- Using
_prefix:_capsys: pytest.CaptureFixture- Actually using the fixture to verify output in the test
As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
🔧 Suggested fix
def test_run_evaluation_success( self, mocker: MockerFixture, - capsys: pytest.CaptureFixture, # pylint: disable=unused-argument + _capsys: pytest.CaptureFixture, ) -> None:tests/unit/pipeline/evaluation/conftest.py-217-223 (1)
217-223: Remove# pylint: disable=redefined-outer-namecomments.Same issue as above - either configure pylint globally for test files or rename the parameters.
🔧 Suggested fix using parameter renaming
`@pytest.fixture` def processor( - config_loader: ConfigLoader, # pylint: disable=redefined-outer-name - processor_components_pr: ProcessorComponents, # pylint: disable=redefined-outer-name + loader: ConfigLoader, + components: ProcessorComponents, ) -> ConversationProcessor: """Create ConversationProcessor instance for PR tests.""" - return ConversationProcessor(config_loader, processor_components_pr) + return ConversationProcessor(loader, components)tests/unit/core/models/test_api_additional.py-34-38 (1)
34-38: Remove# pyright: ignore[reportCallIssue]comment.The coding guidelines prohibit suppressing type checker warnings. This test intentionally passes an invalid argument to verify Pydantic's
extra="forbid"behavior.Consider using
**{"extra_field": "not allowed"}pattern to bypass static type checking without suppression:As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
🔧 Suggested fix
def test_rag_chunk_extra_field_forbidden(self) -> None: """Test that extra fields are forbidden.""" with pytest.raises(ValidationError): - RAGChunk( - content="content", - source="source", - extra_field="not allowed", # pyright: ignore[reportCallIssue] - ) + RAGChunk(**{ + "content": "content", + "source": "source", + "extra_field": "not allowed", + })tests/unit/core/models/test_api_additional.py-105-108 (1)
105-108: Remove# pylint: disable=unsubscriptable-objectcomment.The coding guidelines prohibit inline suppression comments. The assertion on line 103 (
assert request.attachments is not None) should narrow the type, but pylint may not recognize this.Consider using a local variable with explicit type assertion:
🔧 Suggested fix
assert request.attachments is not None assert len(request.attachments) == 2 - assert ( - request.attachments[0].content # pylint: disable=unsubscriptable-object - == "file1" - ) + attachments = request.attachments + assert attachments[0].content == "file1"tests/unit/core/output/conftest.py-80-95 (1)
80-95: Incorrect return type annotation formock_system_config.The fixture returns
config(aMockobject), not theMockerFixture. The return type should reflect the actual return value:🐛 Suggested fix
+from unittest.mock import Mock + + `@pytest.fixture` -def mock_system_config(mocker: MockerFixture) -> MockerFixture: +def mock_system_config(mocker: MockerFixture) -> Mock: """Create mock system config.""" config = mocker.Mock() config.output.enabled_outputs = ["csv", "json", "txt"] config.output.csv_columns = [ "conversation_group_id", "turn_id", "metric_identifier", "result", "score", ] config.visualization.enabled_graphs = [] # Mock model_fields to support iteration in _write_config_params and _build_config_dict config.model_fields.keys.return_value = [] return configtests/unit/core/metrics/test_geval.py-12-12 (1)
12-12: Remove thepylint: disablecomment.Similar to the other test file, consider splitting the test class into smaller, focused classes to avoid exceeding the public methods limit.
As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
tests/unit/core/metrics/test_manager.py-11-11 (1)
11-11: Remove thepylint: disablecomment.The coding guidelines state that lint warnings should not be disabled with
# pylint: disablecomments. Instead, consider splitting the test class into smaller, more focused classes (e.g.,TestMetricManagerResolve,TestMetricManagerThreshold,TestMetricManagerMetadata,TestMetricManagerCount) to reduce the number of public methods.As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
tests/unit/core/metrics/test_geval.py-53-55 (1)
53-55: Removepylint: disable=protected-accesscomments.Multiple test methods disable
protected-accesswarnings when testing private methods like_convert_evaluation_paramsand_get_geval_config. Per coding guidelines, these should be addressed rather than suppressed.Consider one of these approaches:
- Test private methods indirectly through public API
- If direct testing is necessary, make these methods part of a public testing interface
- Use a naming convention like
_test_convert_evaluation_paramsif the project allows testing internalsAs per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
tests/unit/core/metrics/custom/test_tool_eval.py-50-55 (1)
50-55: Removepyright: ignorecomments - usetyping.castorAnyinstead.The coding guidelines prohibit disabling lint warnings with
# type: ignorestyle comments. For intentionally passing invalid types in error-handling tests, usetyping.castor type the test data asAnyto avoid the need for ignores.♻️ Suggested approach
+from typing import Any + ... def test_empty_pattern_match_primary(self) -> None: """Test empty pattern match as primary.""" - expected: list[list[dict]] = [[]] # Primary: no tools expected - actual: list = [] + expected: Any = [[]] # Primary: no tools expected + actual: Any = [] - success, details = evaluate_tool_calls( - expected, actual # pyright: ignore[reportArgumentType] - ) + success, details = evaluate_tool_calls(expected, actual)As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
tests/unit/core/metrics/custom/test_tool_eval.py-92-100 (1)
92-100: Removepyright: ignorecomment - useAnytype for intentionally invalid input.Same issue as above - the test intentionally passes invalid input to verify error handling. Use
Anytype annotation instead of ignoring the type checker.♻️ Suggested fix
def test_error_handling(self) -> None: """Test error handling in evaluate_tool_calls.""" # Invalid expected format should be handled gracefully - expected = "invalid" # Not a list - actual: list = [] + expected: Any = "invalid" # Not a list + actual: Any = [] - success, details = evaluate_tool_calls( - expected, actual # pyright: ignore[reportArgumentType] - ) + success, details = evaluate_tool_calls(expected, actual)As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
tests/unit/core/metrics/custom/test_tool_eval.py-254-261 (1)
254-261: Removepyright: ignorecomment - useAnytype for intentionally invalid input.Same pattern - use
Anytype for testing non-dict arguments behavior instead of type ignores.♻️ Suggested fix
def test_non_dict_arguments(self) -> None: """Test non-dictionary arguments.""" - expected = "not_a_dict" + expected: Any = "not_a_dict" actual = {"key": "value"} - result = _compare_tool_arguments( - expected, actual # pyright: ignore[reportArgumentType] - ) + result = _compare_tool_arguments(expected, actual)As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
🧹 Nitpick comments (12)
tests/unit/core/script/test_manager.py (1)
3-5: Consider alphabetizing standard library imports.Per PEP 8, standard library imports are typically grouped and sorted alphabetically, with
importstatements beforefromimports.♻️ Suggested import order
+import os import tempfile from pathlib import Path -import ostests/conftest.py (1)
6-9: Prefer pytest config over sys.path mutation.
Mutatingsys.pathinconftest.pycan hide import/packaging issues and changes import precedence. Consider configuringpythonpathin pytest config or installing the project in editable mode for tests instead.tests/script/conftest.py (1)
191-211: Consider clarity improvement for result list generation.The
* 5list multiplication works correctly but may be confusing at first glance. Consider using a list comprehension or adding a comment for clarity.Clearer alternative
"results": [ { "conversation_group_id": "conv1", "turn_id": "turn1", "metric_identifier": "ragas:faithfulness", "result": "PASS", "score": 0.95, "threshold": 0.8, "execution_time": 1.0, }, { "conversation_group_id": "conv1", "turn_id": "turn2", "metric_identifier": "ragas:response_relevancy", "result": "PASS", "score": 0.85, "threshold": 0.7, "execution_time": 1.2, }, ] - * 5, # Repeat to get 10 results + * 5, # Repeats the 2-item list 5 times to produce 10 total results }tests/unit/core/config/test_models.py (2)
34-37: Avoid disabling lint warnings with inline comments.The
# pylint: disable=unsubscriptable-objectcomments on Lines 35, 72, 76, 80 violate the coding guidelines. The underlying issue should be fixed instead of suppressing the warning.Consider simplifying the assertions to avoid the subscript issue:
♻️ Suggested refactor
- assert ( - turn.contexts[0] # pylint: disable=unsubscriptable-object - == "Python context" - ) + assert turn.contexts[0] == "Python context"If pylint raises
unsubscriptable-objecton a valid subscript operation (e.g., becausecontextsis typed asOptional[list[str]]), the cleaner fix is to assert the type or use a guard before subscripting, which you already do withassert turn.contexts is not Noneon Line 32.As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead."
Also applies to: 71-82
54-56: Avoidpyright: ignorecomments.The
# pyright: ignore[reportArgumentType]comment should be removed per coding guidelines. If the test intentionally passes an invalid type to trigger validation, consider usingtyping.castor restructuring the test data.♻️ Suggested approach
- contexts=[ - {"title": "No content field"} - ], # pyright: ignore[reportArgumentType] + contexts=[ + {"title": "No content field"} # type: ignore[arg-type] + ],Actually, since
# type: ignoreis also discouraged, the better approach is to usetyping.Anyor restructure the test:from typing import Any invalid_contexts: Any = [{"title": "No content field"}] TurnData( turn_id="1", query="Valid query", response="Valid response", contexts=invalid_contexts, )As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead."
tests/unit/core/models/test_system_additional.py (1)
199-199: Avoidpyright: ignorecomment.The
# pyright: ignore[reportArgumentType]comment violates coding guidelines. Since Pydantic'sfigsizefield expectslist[int]but you're passing a tuple, consider using a list literal instead:♻️ Suggested fix
- figsize=(12, 8), # pyright: ignore[reportArgumentType] + figsize=[12, 8],As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead."
tests/unit/pipeline/evaluation/test_evaluator.py (3)
431-444: Consider restructuring to avoid# pylint: disable=protected-access.Multiple assertions use
# pylint: disable=protected-accessto test_determine_status. While testing protected methods is sometimes necessary, the inline disable comments violate coding guidelines.Consider one of these approaches:
- Test the behavior through the public
evaluate_metricmethod instead- If direct testing is required, extract the disable to a class-level or file-level comment (though still discouraged)
- Expose the method for testing via a different pattern
As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead."
Also applies to: 474-481
503-506: Add proper type hints tocreate_mock_handlerinstead of using# type: ignore.The
# type: ignore[no-untyped-def]comment should be replaced with proper type annotations:♻️ Suggested fix
- def create_mock_handler( # type: ignore[no-untyped-def] - mocker: MockerFixture, - mock_return: tuple[float, str] | list[tuple[float, str]], - ): + def create_mock_handler( + mocker: MockerFixture, + mock_return: tuple[float, str] | list[tuple[float, str]], + ) -> mocker.Mock:Or if the return type is complex:
from typing import Any def create_mock_handler( mocker: MockerFixture, mock_return: tuple[float, str] | list[tuple[float, str]], ) -> Any:As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead."
483-483: Consider refactoring to reduce method arguments instead of disabling pylint.The
# pylint: disable=too-many-arguments, too-many-positional-argumentscomments appear on helper methods. Consider using a dataclass or named tuple to group related parameters, or restructuring the test setup.As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead."
Also applies to: 561-561, 685-685
tests/unit/core/models/test_data.py (2)
45-45: Removepyright: ignorecomments.Multiple lines use
# pyright: ignore[reportArgumentType]to pass intentionally invalid types for validation testing. Consider usingtyping.Anyortyping.castto handle these cases:♻️ Suggested approach
from typing import Any # For intentionally invalid input in tests: invalid_input: Any = "not_a_list" TurnData( turn_id="test_invalid", query="Test query", expected_tool_calls=invalid_input, )As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead."
Also applies to: 168-168, 232-232, 329-332
53-65: Remove# pylint: disable=unsubscriptable-objectcomments.These assertions access nested list elements after confirming the parent is not None. The pylint warning can be avoided by:
- Using intermediate variables with type narrowing
- Adding explicit type assertions
♻️ Example approach
expected = turn_data.expected_tool_calls assert expected is not None first_alt = expected[0] # Type narrowed after assert assert len(first_alt) == 1 assert first_alt[0][0]["tool_name"] == "test_tool"As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead."
Also applies to: 82-89, 105-110, 129-134
tests/unit/core/metrics/test_geval.py (1)
3-3: Use pytestmockerfixture instead ofunittest.mock.The coding guidelines specify using pytest mocking with the
mockerfixture rather than importing fromunittest.mock.♻️ Suggested refactor
-from unittest.mock import MagicMock, patch +from pytest_mock import MockerFixtureThen update fixtures and tests to use
mocker.MagicMock()andmocker.patch()instead.As per coding guidelines: "Use pytest mocking with mocker fixture instead of unittest.mock"
| result = comparison_instance._compare_score_distributions( # pylint: disable=protected-access | ||
| scores1, scores2 | ||
| ) |
There was a problem hiding this comment.
Remove # pylint: disable=protected-access and test via public APIs or helpers.
Line 147 and other marked sites bypass lint rules; please replace with public entry points or add a small test helper/wrapper in the module instead of suppressing warnings. As per coding guidelines: Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead.
Also applies to: 183-185, 203-205, 227-229, 255-257, 292-294, 306-308, 322-324, 335-337, 348-350, 363-365, 397-399, 428-430, 453-455, 470-472, 501-503
🤖 Prompt for AI Agents
In `@tests/script/test_compare_evaluations.py` around lines 147 - 149, The tests
are calling the protected method _compare_score_distributions (and several other
protected methods) via a pylint disable; instead add or use a public API/wrapper
in the module (e.g., a new public helper function compare_score_distributions or
a test-only wrapper function) that delegates to
comparison_instance._compare_score_distributions, update the tests to call that
public helper instead of the protected method, and remove the "# pylint:
disable=protected-access" comments; ensure the helper is exported from the
module where Comparison/ComparisonInstance is defined so tests can import it
without accessing protected members.
| import sys | ||
| from pathlib import Path | ||
| from typing import Any | ||
| from unittest.mock import patch |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -path "*/tests/script/test_run_multi_provider_eval.py" -type fRepository: lightspeed-core/lightspeed-evaluation
Length of output: 136
🏁 Script executed:
cd /repo && cat -n tests/script/test_run_multi_provider_eval.py | head -50Repository: lightspeed-core/lightspeed-evaluation
Length of output: 136
🏁 Script executed:
git ls-files | grep -i "test_run_multi_provider_eval"Repository: lightspeed-core/lightspeed-evaluation
Length of output: 125
🏁 Script executed:
pwd && git ls-files tests/script/test_run_multi_provider_eval.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 144
🏁 Script executed:
cat -n tests/script/test_run_multi_provider_eval.py | head -80Repository: lightspeed-core/lightspeed-evaluation
Length of output: 3796
🏁 Script executed:
cat -n tests/script/test_run_multi_provider_eval.py | grep -E "(patch|pylint|disable)" -A 2 -B 2Repository: lightspeed-core/lightspeed-evaluation
Length of output: 167
🏁 Script executed:
wc -l tests/script/test_run_multi_provider_eval.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 129
🏁 Script executed:
rg "patch\(" tests/script/test_run_multi_provider_eval.py -nRepository: lightspeed-core/lightspeed-evaluation
Length of output: 320
🏁 Script executed:
rg "pylint|noqa|type: ignore" tests/script/test_run_multi_provider_eval.py -nRepository: lightspeed-core/lightspeed-evaluation
Length of output: 167
🏁 Script executed:
rg "^[[:space:]]*#.*pylint" tests/script/test_run_multi_provider_eval.py -nRepository: lightspeed-core/lightspeed-evaluation
Length of output: 63
🏁 Script executed:
sed -n '415,425p' tests/script/test_run_multi_provider_eval.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 496
🏁 Script executed:
sed -n '478,515p' tests/script/test_run_multi_provider_eval.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 1744
🏁 Script executed:
sed -n '536,580p' tests/script/test_run_multi_provider_eval.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 1988
🏁 Script executed:
rg "pytest-mock|pytest_mock" -g "*.txt" -g "*.cfg" -g "*.toml" -g "*.yaml" -g "*.yml" -g "*.in"Repository: lightspeed-core/lightspeed-evaluation
Length of output: 339
🏁 Script executed:
cat pyproject.toml | grep -A 20 "dependencies\|pytest"Repository: lightspeed-core/lightspeed-evaluation
Length of output: 2576
🏁 Script executed:
sed -n '536,560p' tests/script/test_run_multi_provider_eval.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 1091
Replace unittest.mock.patch with pytest mocker fixture and remove # pylint: disable comments.
Line 7 imports unittest.mock.patch, but the codebase uses pytest mocker fixture instead (pytest-mock is available in dependencies). Additionally, the file contains multiple # pylint: disable=protected-access comments throughout, which violates the coding guideline against disabling lint warnings.
Please:
- Replace
from unittest.mock import patchwith pytest mocker fixture usage - Remove all
# pylint: disable=protected-accesscomments and refactor the code to avoid protected member access (e.g., use public methods or test fixtures instead of calling_run_single_evaluationdirectly) - Update test method signatures to accept
mocker: MockerFixtureparameter and usemocker.patch()instead ofwith patch()context managers
🔧 Example pattern
-from unittest.mock import patch
+from pytest_mock import MockerFixture
-def test_run_single_evaluation_success(self, runner: MultiProviderEvaluationRunner) -> None:
+def test_run_single_evaluation_success(
+ self, runner: MultiProviderEvaluationRunner, mocker: MockerFixture
+) -> None:
- with patch("script.run_multi_provider_eval.run_evaluation", return_value={"PASS": 5, "FAIL": 2, "ERROR": 0}):
+ mocker.patch(
+ "script.run_multi_provider_eval.run_evaluation",
+ return_value={"PASS": 5, "FAIL": 2, "ERROR": 0},
+ )🤖 Prompt for AI Agents
In `@tests/script/test_run_multi_provider_eval.py` at line 7, Replace the
unittest.patch import and context managers with the pytest-mock fixture: remove
"from unittest.mock import patch", update tests to accept a "mocker:
MockerFixture" parameter, and replace "with patch(...)" uses by
"mocker.patch(...)" (or "mocker.patch.object(...)") in each test. Remove all "#
pylint: disable=protected-access" comments and refactor tests that call
protected members (e.g., "_run_single_evaluation") to use public APIs or test
helpers instead (call the public method that triggers the evaluation flow or
expose a test fixture/wrapper that invokes the internal behavior). Ensure any
patch targets reference the same symbols used in the diff (function/class names)
so mocks still apply after converting to mocker.patch.
| config = runner._load_yaml( # pylint: disable=protected-access | ||
| temp_config_files["providers_config"] | ||
| ) |
There was a problem hiding this comment.
Remove # pylint: disable=... comments and address the underlying lint concerns.
These suppressions (protected-access, too-few-public-methods, consider-using-with) violate the repo’s linting policy. Prefer testing through public APIs or introducing a public test hook; for the temp-file helper, use a proper context manager rather than disabling the warning. As per coding guidelines: Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead.
Also applies to: 262-263, 273-274, 284-285, 315-318, 352-355, 376-379, 410-412, 427-430, 445-448, 486-490, 513-516, 543-547, 563-567, 581-585, 592-593, 617-618, 678-680, 689-691, 695-697, 276-276
🤖 Prompt for AI Agents
In `@tests/script/test_run_multi_provider_eval.py` around lines 244 - 246, The
tests currently suppress linting (e.g., protected-access,
too-few-public-methods, consider-using-with) around calls like runner._load_yaml
and the temp file helper in temp_config_files; remove those # pylint: disable
comments and fix the root causes by (1) using the public API instead of calling
the protected method runner._load_yaml (expose a public runner.load_yaml or add
a test-only public hook method), and (2) replacing the temp_config_files usage
with a proper context-manager based temp file helper (or use pytest
tmp_path/tmp_path_factory) so files are opened/closed safely; apply the same
changes wherever similar disables are present in this test file by switching to
public methods or adding minimal test hooks and context-managed temp file usage.
| def test_parse_tool_call_invalid_structure(self) -> None: | ||
| """Test parsing malformed tool call.""" | ||
| token = "not a dict" | ||
|
|
||
| result = _parse_tool_call(token) | ||
| result = _parse_tool_call(token) # pyright: ignore[reportArgumentType] |
There was a problem hiding this comment.
Remove the lint suppression and make the invalid-type test type-safe.
Line 231 uses a pyright ignore, which is disallowed. You can keep the invalid-structure test while avoiding the suppression by typing the token as Any.
🔧 Suggested fix
def test_parse_tool_call_invalid_structure(self) -> None:
"""Test parsing malformed tool call."""
- token = "not a dict"
-
- result = _parse_tool_call(token) # pyright: ignore[reportArgumentType]
+ token: Any = "not a dict"
+ result = _parse_tool_call(token)As per coding guidelines, "**/*.py: Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead".
🤖 Prompt for AI Agents
In `@tests/unit/core/api/test_streaming_parser.py` around lines 227 - 231, The
test test_parse_tool_call_invalid_structure uses a pyright ignore; instead make
the token variable type-safe by importing Any from typing and declaring token:
Any = "not a dict", remove the "# pyright: ignore" comment, and keep the rest of
the test calling _parse_tool_call(token) so the invalid-structure behavior is
exercised without disabling linting; locate the assignment in
test_parse_tool_call_invalid_structure and update the declaration accordingly.
tests/unit/core/metrics/conftest.py
Outdated
|
|
||
|
|
||
| @pytest.fixture | ||
| def sample_scope( # pylint: disable=redefined-outer-name |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/unit/core/metrics/conftest.py | head -100Repository: lightspeed-core/lightspeed-evaluation
Length of output: 3506
Remove redefined-outer-name pylint suppressions. Rename the sample_turn_data fixture function with an underscore prefix and use the name= parameter in the @pytest.fixture() decorator to expose it with the original name. This eliminates the parameter name shadowing that triggers the warning, avoiding the need for pylint disables per coding guidelines: "**/*.py: Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead".
🛠️ Suggested refactor to avoid pylint disables
`@pytest.fixture`
-def sample_turn_data() -> TurnData:
+def _sample_turn_data() -> TurnData:
"""Create sample TurnData for testing."""
return TurnData(
turn_id="test_turn",
query="What is the capital of France?",
response="The capital of France is Paris.",
expected_response="The capital of France is Paris.",
)
+
+
+@pytest.fixture(name="sample_turn_data")
+def _(sample_turn_data: TurnData) -> TurnData:
+ return sample_turn_data
`@pytest.fixture`
-def sample_scope( # pylint: disable=redefined-outer-name
+def sample_scope(
sample_turn_data: TurnData,
) -> EvaluationScope:
`@pytest.fixture`
-def conversation_scope( # pylint: disable=redefined-outer-name
+def conversation_scope(
sample_turn_data: TurnData,
) -> EvaluationScope:Also applies to: 82-82
🤖 Prompt for AI Agents
In `@tests/unit/core/metrics/conftest.py` at line 70, The fixtures currently use
pylint disable comments to suppress redefined-outer-name (e.g., the sample_scope
fixture and the sample_turn_data fixture); rename the actual fixture function to
have a leading underscore (e.g., _sample_turn_data) and change the decorator to
`@pytest.fixture`(name="sample_turn_data") so the fixture is still exposed with
the original name but no longer shadows an outer name; update any other similar
fixtures (the one referenced at lines ~82) the same way and remove the
corresponding "# pylint: disable=redefined-outer-name" comments.
| ] | ||
|
|
||
| stats = handler._calculate_stats(results) | ||
| stats = handler._calculate_stats(results) # pylint: disable=protected-access |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/unit/core/output/test_final_coverage.py | head -140 | tail -50Repository: lightspeed-core/lightspeed-evaluation
Length of output: 2074
🏁 Script executed:
wc -l tests/unit/core/output/test_final_coverage.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 130
🏁 Script executed:
cat -n tests/unit/core/output/test_final_coverage.py | sed -n '95,105p'Repository: lightspeed-core/lightspeed-evaluation
Length of output: 468
🏁 Script executed:
cat -n tests/unit/core/output/test_final_coverage.py | sed -n '120,130p'Repository: lightspeed-core/lightspeed-evaluation
Length of output: 446
🏁 Script executed:
fd -t f "output.*\.py" src/ | head -20Repository: lightspeed-core/lightspeed-evaluation
Length of output: 63
🏁 Script executed:
rg -l "class OutputHandler" --type pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 131
🏁 Script executed:
rg -A 30 "class OutputHandler" --type pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 2754
🏁 Script executed:
rg "_calculate_stats|_generate_csv_report" src/lightspeed_evaluation/core/output/generator.py -A 5Repository: lightspeed-core/lightspeed-evaluation
Length of output: 1206
🏁 Script executed:
rg "def .*calculate_stats|def .*generate_csv_report" src/lightspeed_evaluation/core/output/generator.py -B 2 -A 10Repository: lightspeed-core/lightspeed-evaluation
Length of output: 1090
Remove protected-access lint suppressions and test via public APIs.
The test directly calls private methods _calculate_stats() (line 100) and _generate_csv_report() (line 126) which triggers the protected-access warning. Instead of suppressing the warning, refactor the tests to validate behavior through the public generate_reports() API, or if these methods require independent testing, make them public by removing the leading underscore. Per coding guidelines, fix the underlying issue rather than disabling lint warnings.
🤖 Prompt for AI Agents
In `@tests/unit/core/output/test_final_coverage.py` at line 100, The test
currently calls private methods _calculate_stats and _generate_csv_report
directly; remove the protected-access suppression by either (A) exercising the
public API generate_reports() in the test and asserting the resulting
outputs/side-effects that cover the same logic, or (B) if independent unit tests
are required, make those methods public by renaming _calculate_stats ->
calculate_stats and _generate_csv_report -> generate_csv_report (and update any
call sites), then update tests to call the new public methods; ensure assertions
validate the same behavior originally checked by the private calls.
|
|
||
|
|
||
| class TestSystemLoaderEdgeCases: | ||
| class TestSystemLoaderEdgeCases: # pylint: disable=too-few-public-methods |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the test file
find . -name "test_final_coverage.py" -path "*/tests/unit/core/output/*"Repository: lightspeed-core/lightspeed-evaluation
Length of output: 128
🏁 Script executed:
# Check the file and read around line 137
if [ -f "tests/unit/core/output/test_final_coverage.py" ]; then
wc -l "tests/unit/core/output/test_final_coverage.py"
fiRepository: lightspeed-core/lightspeed-evaluation
Length of output: 130
🏁 Script executed:
# Read the specific class definition
sed -n '130,160p' tests/unit/core/output/test_final_coverage.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 1126
🏁 Script executed:
# Get the full class definition to see if there are more methods
sed -n '137,159p' tests/unit/core/output/test_final_coverage.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 934
Remove the pylint disable comment and restructure the test.
The class has only one test method, which triggers the too-few-public-methods warning. Per coding guidelines, do not disable lint warnings—fix the underlying issue instead. Either convert the test to a module-level function or add related edge case tests to the class.
🤖 Prompt for AI Agents
In `@tests/unit/core/output/test_final_coverage.py` at line 137, Remove the pylint
disable by addressing the underlying issue in class TestSystemLoaderEdgeCases:
either convert the class into a module-level test function (rename the single
test method to a top-level function and remove the class wrapper) or add at
least one more related test method covering another edge case for the same
behavior so the class legitimately contains multiple public tests; then delete
the "# pylint: disable=too-few-public-methods" comment and run the test/lint
suite to confirm the warning is gone.
| """Test statistics calculation.""" | ||
| handler = OutputHandler(output_dir=str(tmp_path)) | ||
| stats = handler._calculate_stats(sample_results) | ||
| stats = handler._calculate_stats( # pylint: disable=protected-access |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test file structure
wc -l tests/unit/core/output/test_generator.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 125
🏁 Script executed:
# Get an overview of the test file around the flagged lines
head -100 tests/unit/core/output/test_generator.py | cat -nRepository: lightspeed-core/lightspeed-evaluation
Length of output: 3982
🏁 Script executed:
# Let's also search for the _calculate_stats method and handler class definition
rg "_calculate_stats|class.*Handler" tests/unit/core/output/test_generator.py -A 3 -B 1Repository: lightspeed-core/lightspeed-evaluation
Length of output: 2154
🏁 Script executed:
# Find the handler class to understand its public API
find . -name "*.py" -path "*/output/*" -type f | grep -v __pycache__ | grep -v testRepository: lightspeed-core/lightspeed-evaluation
Length of output: 356
🏁 Script executed:
# Read the OutputHandler class to understand its structure
cat -n ./src/lightspeed_evaluation/core/output/generator.py | head -200Repository: lightspeed-core/lightspeed-evaluation
Length of output: 8620
🏁 Script executed:
# Search for public methods in the OutputHandler class
rg "def " ./src/lightspeed_evaluation/core/output/generator.py | head -30Repository: lightspeed-core/lightspeed-evaluation
Length of output: 1153
🏁 Script executed:
# Let's see the full test file to understand the test assertions better
sed -n '100,300p' tests/unit/core/output/test_generator.py | cat -nRepository: lightspeed-core/lightspeed-evaluation
Length of output: 8525
🏁 Script executed:
# Check if there are any integration tests that use the public API
rg "generate_reports" tests/ -B 3 -A 5Repository: lightspeed-core/lightspeed-evaluation
Length of output: 4408
Refactor tests to use the public generate_reports() API instead of accessing private methods.
The OutputHandler class provides a public interface via generate_reports() which internally orchestrates all report generation. Tests should exercise this public API and verify outputs through assertions on generated files, rather than directly accessing protected methods like _calculate_stats(), _generate_csv_report(), _generate_json_summary(), and _generate_text_summary().
Existing tests (test_generate_reports_creates_files, test_generate_reports_with_empty_results, test_generate_reports_without_config) demonstrate the correct pattern—calling the public method and asserting on output file presence and content.
Remove all # pylint: disable=protected-access comments and refactor affected tests accordingly.
Also applies to: 41-41, 58-58, 75-75, 85-85, 111-111, 121-121, 205-205, 209-209, 228-228, 232-232, 251-251, 254-254, 282-282, 412-412, 464-464, 490-490
🤖 Prompt for AI Agents
In `@tests/unit/core/output/test_generator.py` at line 29, Replace direct calls to
protected methods (e.g., OutputHandler._calculate_stats, _generate_csv_report,
_generate_json_summary, _generate_text_summary) in the tests with calls to the
public OutputHandler.generate_reports() API; remove all corresponding "# pylint:
disable=protected-access" comments and instead assert on the generated output
files and their contents (as done in test_generate_reports_creates_files,
test_generate_reports_with_empty_results, test_generate_reports_without_config)
to verify behavior. Ensure each affected test constructs the same handler
input/state, invokes handler.generate_reports(), then checks for expected
files/contents rather than inspecting internal helper methods.
| self, | ||
| tmp_path: Path, | ||
| sample_results: list[EvaluationResult], | ||
| mock_system_config: MockerFixture, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the fixture definition and usages of mock_system_config
rg -n "def mock_system_config|mock_system_config" tests/unit/core/output/test_generator.py -A 2 -B 1Repository: lightspeed-core/lightspeed-evaluation
Length of output: 949
🏁 Script executed:
#!/bin/bash
# Check the conftest file(s) in the tests directory for fixture definitions
fd conftest.py tests/ -x rg -l "mock_system_config" {}Repository: lightspeed-core/lightspeed-evaluation
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Search for mock_system_config fixture definition across test directories
rg -n "def mock_system_config" tests/ -A 3Repository: lightspeed-core/lightspeed-evaluation
Length of output: 419
🏁 Script executed:
#!/bin/bash
# Find SystemConfig definition and CSVGenerator usage
rg -n "class SystemConfig|SystemConfig" --type py | head -20Repository: lightspeed-core/lightspeed-evaluation
Length of output: 1784
🏁 Script executed:
#!/bin/bash
# Check CSVGenerator constructor to see what type it expects for system_config
rg -n "class CSVGenerator|def __init__|system_config" tests/unit/core/output/test_generator.py -B 2 -A 5 | head -30Repository: lightspeed-core/lightspeed-evaluation
Length of output: 1103
🏁 Script executed:
#!/bin/bash
# Find the actual generator implementation
fd -name "generator.py" -path "*/output/*" ! -path "*/tests/*" -x cat {} \; | head -60Repository: lightspeed-core/lightspeed-evaluation
Length of output: 311
🏁 Script executed:
#!/bin/bash
# Find the actual generator module in src/
fd -name "generator.py" ! -path "*/tests/*"Repository: lightspeed-core/lightspeed-evaluation
Length of output: 311
🏁 Script executed:
#!/bin/bash
# Check the OutputHandler class to see what type system_config should be
rg -n "class OutputHandler" -A 15 ! -path "*/tests/*"Repository: lightspeed-core/lightspeed-evaluation
Length of output: 172
🏁 Script executed:
#!/bin/bash
# Find SystemConfig import and definition in the main source
rg -n "class SystemConfig" ! -path "*/tests/*" -A 5Repository: lightspeed-core/lightspeed-evaluation
Length of output: 172
🏁 Script executed:
#!/bin/bash
# Find the actual generator module in src/
find . -name "generator.py" -not -path "*/tests/*"Repository: lightspeed-core/lightspeed-evaluation
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Check the OutputHandler class to see what type system_config should be
rg "class OutputHandler" -A 15 --type py | grep -v testsRepository: lightspeed-core/lightspeed-evaluation
Length of output: 1443
🏁 Script executed:
#!/bin/bash
# Find SystemConfig import and definition in the main source
rg "class SystemConfig" -A 5 --type py | head -40Repository: lightspeed-core/lightspeed-evaluation
Length of output: 548
🏁 Script executed:
#!/bin/bash
# Check the full conftest.py fixture definition
cat -n tests/unit/core/output/conftest.py | sed -n '81,90p'Repository: lightspeed-core/lightspeed-evaluation
Length of output: 487
🏁 Script executed:
#!/bin/bash
# Check imports in conftest.py to understand MockerFixture usage
head -30 tests/unit/core/output/conftest.pyRepository: lightspeed-core/lightspeed-evaluation
Length of output: 1001
Fix type annotation: mock_system_config should not be MockerFixture.
The fixture returns mocker.Mock() (a mock object), not a MockerFixture. Update the fixture's return type annotation in conftest.py line 81 and the parameter annotations at lines 50, 147, and 173-174 to a more accurate type like MagicMock or Any. MockerFixture is the type of the mocker fixture parameter, not the return value of mocker.Mock().
🤖 Prompt for AI Agents
In `@tests/unit/core/output/test_generator.py` at line 50, The type annotation is
incorrect: the fixture mock_system_config returns a mock object (mocker.Mock()),
not a MockerFixture; update the fixture signature named mock_system_config in
conftest and all test parameter annotations (the mock_system_config parameters
in test_generator.py) to use a more accurate type such as
unittest.mock.MagicMock or typing.Any (replace MockerFixture with MagicMock/Any)
so the declared return type matches the actual mock object returned by
mocker.Mock().
| result = ( | ||
| validator._validate_evaluation_data( # pylint: disable=protected-access | ||
| [conv_data] | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Remove pylint: disable=protected-access and test via public API.
Please avoid suppressing lint checks; prefer exercising public methods or make the helper methods public if they are part of the intended test surface. As per coding guidelines, **/*.py: Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead.
Also applies to: 87-91, 117-121, 141-145, 163-167, 192-196, 215-219, 241-245, 275-278, 376-380, 397-401, 415-419, 450-454, 477-477, 500-502, 523-525, 549-551, 564-566
🤖 Prompt for AI Agents
In `@tests/unit/core/system/test_validator.py` around lines 58 - 62, The test
currently calls the internal helper validator._validate_evaluation_data with a
pylint disable; remove the protected-access suppression and test via the public
API instead (e.g., call validator.validate_evaluation or whichever public method
exercises the same validation path) or, if this helper is intended to be part of
the test surface, make it public (rename _validate_evaluation_data to
validate_evaluation_data) and update tests to call the public name; apply the
same change for the other occurrences listed so tests no longer use pylint:
disable=protected-access.
VladimirKadlec
left a comment
VladimirKadlec
left a comment
There was a problem hiding this comment.
I suggest disabling specific pylint checks (that don't make sense for tests) globally at the beginning of the file just once. For example: # pylint: disable=protected-access
Otherwise LGTM 💪 , thank you
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/pipeline/evaluation/test_evaluator.py (1)
490-499: Remove# type: ignorecomment and add proper type annotation.The
create_mock_handlerfunction has a# type: ignore[no-untyped-def]comment which violates coding guidelines. The function can be properly typed:Proposed fix
- def create_mock_handler( # type: ignore[no-untyped-def] - mocker: MockerFixture, - mock_return: tuple[float, str] | list[tuple[float, str]], - ): + def create_mock_handler( + mocker: MockerFixture, + mock_return: tuple[float, str] | list[tuple[float, str]], + ) -> mocker.Mock:Note: If
mocker.Mockdoesn't work as a return type, useunittest.mock.Mockfrom the standard library orAnyas a fallback.As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
🤖 Fix all issues with AI agents
In `@tests/unit/core/api/test_client.py`:
- Line 1: Remove the module-level "# pylint: disable=protected-access" and
refactor tests in tests/unit/core/api/test_client.py to stop accessing protected
members directly; update tests to exercise behavior via the public API (e.g.,
use APIClient.query, APIClient.request, or APIClient.execute) or, if truly
required, add a safe public helper on APIClient (for example get_internal_state
or debug_inspect) and test that instead; then delete the disable comment and
ensure all assertions use those public methods.
In `@tests/unit/core/llm/test_custom.py`:
- Line 1: Remove the file-level pylint disables and fix the underlying issues:
replace the test class with standalone pytest functions (remove the class that
references the private attribute) and update the test to exercise the public API
instead of accessing tracker._token_callback (or add a small public helper
method on the Tracker class, e.g., Tracker.token_callback or
Tracker.invoke_token_callback, and use that in tests). Alternatively, if
class-style tests are required, remove the disable and add a project-level
pylint config to allow test classes, but preferred is converting tests to
top-level functions and using the public Tracker API rather than accessing the
private attribute.
In `@tests/unit/core/metrics/test_manager.py`:
- Line 1: The test module currently suppresses pylint with "# pylint:
disable=too-many-public-methods"; split the large TestMetricManager into
smaller, focused test classes or module-level test functions (e.g.,
TestMetricManagerRegister, TestMetricManagerUpdate, TestMetricManagerDelete or
individual test_* functions grouped by feature) so each class has few public
methods, remove the pylint disable comment, and keep existing test names and
assertions intact; update any shared fixtures or setup/teardown used by
TestMetricManager into module-level fixtures or a small base fixture/helper to
avoid duplication while ensuring all tests still import and use the same
helpers.
In `@tests/unit/pipeline/evaluation/test_evaluator.py`:
- Around line 1-2: Remove the file-level "# pylint:
disable=protected-access,redefined-outer-name,too-many-arguments,too-many-positional-arguments"
directive from test_evaluator.py and fix each linter issue instead: rename any
fixtures that shadow outer names to avoid redefined-outer-name, refactor tests
with many parameters into smaller tests or compose fixtures to address
too-many-arguments/too-many-positional-arguments, and stop directly calling
protected members (replace with public API calls or test behavior via public
interfaces) rather than suppressing protected-access; if a project-wide
exception for testing protected members is required, make that change centrally
in the test lint/config (not via in-file disables).
In `@tests/unit/pipeline/evaluation/test_processor.py`:
- Around line 1-2: Remove the file-level "# pylint: disable=..." directive and
instead address each lint issue: remove truly unused parameters (e.g., drop
unused "mocker" arguments from individual test function signatures), refactor
tests with many parameters (split into fixtures or a test helper/class used by
the test functions in this file) to eliminate
too-many-arguments/too-many-positional-arguments warnings, and for legitimate
protected access in tests (calls to private methods) add a project-wide
test-file exception in pylintrc or move the single-line disable to the specific
test that accesses the protected member; update the tests in this module
accordingly so no file-level disable is required.
In `@tests/unit/runner/test_evaluation.py`:
- Around line 1-2: Remove the file-level "# pylint: disable=unused-argument"
from tests/unit/runner/test_evaluation.py and fix the underlying unused-argument
issues by either deleting parameters that are not used in the test functions or
renaming intentionally unused fixture parameters to start with an underscore
(e.g., change "fixture" to "_fixture") so they remain for side-effects; scan all
test functions in test_evaluation.py for unused parameters and apply the same
pattern (match what was done in test_processor.py) to satisfy lint rules without
suppressions.
| @@ -1,45 +1,23 @@ | |||
| # pylint: disable=protected-access | |||
There was a problem hiding this comment.
Remove the pylint disable and avoid protected-access tests.
Line 1 disables protected-access, which violates the guideline to fix underlying issues instead of suppressing lint. Please refactor these tests to exercise behavior via public APIs (e.g., APIClient.query) or adjust production code to expose safe, public helpers, then remove the disable.
As per coding guidelines: Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead.
🤖 Prompt for AI Agents
In `@tests/unit/core/api/test_client.py` at line 1, Remove the module-level "#
pylint: disable=protected-access" and refactor tests in
tests/unit/core/api/test_client.py to stop accessing protected members directly;
update tests to exercise behavior via the public API (e.g., use APIClient.query,
APIClient.request, or APIClient.execute) or, if truly required, add a safe
public helper on APIClient (for example get_internal_state or debug_inspect) and
test that instead; then delete the disable comment and ensure all assertions use
those public methods.
| @@ -1,6 +1,9 @@ | |||
| # pylint: disable=protected-access,disable=too-few-public-methods | |||
There was a problem hiding this comment.
Pylint disable comment violates coding guidelines.
The coding guidelines state: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead."
Consider addressing the underlying issues:
protected-access: Line 25 accessestracker._token_callback. Either expose this as a public method for testing, or test through the public interface only.too-few-public-methods: This is typical for pytest test classes. Consider using standalone test functions instead of classes, or configure pylint to ignore this for test files at the project level (e.g., inpyproject.tomlor.pylintrc).
As per coding guidelines: **/*.py: Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead.
🤖 Prompt for AI Agents
In `@tests/unit/core/llm/test_custom.py` at line 1, Remove the file-level pylint
disables and fix the underlying issues: replace the test class with standalone
pytest functions (remove the class that references the private attribute) and
update the test to exercise the public API instead of accessing
tracker._token_callback (or add a small public helper method on the Tracker
class, e.g., Tracker.token_callback or Tracker.invoke_token_callback, and use
that in tests). Alternatively, if class-style tests are required, remove the
disable and add a project-level pylint config to allow test classes, but
preferred is converting tests to top-level functions and using the public
Tracker API rather than accessing the private attribute.
| @@ -1,6 +1,6 @@ | |||
| """Unit tests for core metrics manager module.""" | |||
| # pylint: disable=too-many-public-methods | |||
There was a problem hiding this comment.
Remove the too-many-public-methods suppression by splitting the test class.
Consider breaking TestMetricManager into smaller focused classes (or module-level test functions) and drop the pylint disable. As per coding guidelines: "**/*.py: Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead."
🤖 Prompt for AI Agents
In `@tests/unit/core/metrics/test_manager.py` at line 1, The test module currently
suppresses pylint with "# pylint: disable=too-many-public-methods"; split the
large TestMetricManager into smaller, focused test classes or module-level test
functions (e.g., TestMetricManagerRegister, TestMetricManagerUpdate,
TestMetricManagerDelete or individual test_* functions grouped by feature) so
each class has few public methods, remove the pylint disable comment, and keep
existing test names and assertions intact; update any shared fixtures or
setup/teardown used by TestMetricManager into module-level fixtures or a small
base fixture/helper to avoid duplication while ensuring all tests still import
and use the same helpers.
| # pylint: disable=protected-access,redefined-outer-name,too-many-arguments,too-many-positional-arguments | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove file-level pylint disable directive.
Same issue as other test files. The coding guidelines prohibit # pylint: disable comments. Consider:
protected-access: Could be allowed project-wide for test files testing private methodsredefined-outer-name: Often indicates fixture shadowing - rename fixtures to avoid conflictstoo-many-arguments/too-many-positional-arguments: Refactor with fixture composition
As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
🤖 Prompt for AI Agents
In `@tests/unit/pipeline/evaluation/test_evaluator.py` around lines 1 - 2, Remove
the file-level "# pylint:
disable=protected-access,redefined-outer-name,too-many-arguments,too-many-positional-arguments"
directive from test_evaluator.py and fix each linter issue instead: rename any
fixtures that shadow outer names to avoid redefined-outer-name, refactor tests
with many parameters into smaller tests or compose fixtures to address
too-many-arguments/too-many-positional-arguments, and stop directly calling
protected members (replace with public API calls or test behavior via public
interfaces) rather than suppressing protected-access; if a project-wide
exception for testing protected members is required, make that change centrally
in the test lint/config (not via in-file disables).
| # pylint: disable=unused-argument,protected-access,too-many-arguments, too-many-positional-arguments | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove file-level pylint disable directive and fix underlying issues.
The coding guidelines explicitly state not to disable lint warnings with # pylint: disable comments. Instead:
unused-argument: Parameters likemockerthat appear unused are often needed for fixture side-effects; if truly unused, remove them from the signatureprotected-access: Acceptable in tests for testing private methods; consider adding a project-wide exception for test filestoo-many-arguments/too-many-positional-arguments: Refactor using fixture composition or test helper classes
As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
🤖 Prompt for AI Agents
In `@tests/unit/pipeline/evaluation/test_processor.py` around lines 1 - 2, Remove
the file-level "# pylint: disable=..." directive and instead address each lint
issue: remove truly unused parameters (e.g., drop unused "mocker" arguments from
individual test function signatures), refactor tests with many parameters (split
into fixtures or a test helper/class used by the test functions in this file) to
eliminate too-many-arguments/too-many-positional-arguments warnings, and for
legitimate protected access in tests (calls to private methods) add a
project-wide test-file exception in pylintrc or move the single-line disable to
the specific test that accesses the protected member; update the tests in this
module accordingly so no file-level disable is required.
| # pylint: disable=unused-argument | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove file-level pylint disable directive.
Same issue as in test_processor.py - the coding guidelines prohibit # pylint: disable comments. The unused-argument warnings should be addressed by either:
- Removing truly unused parameters from test signatures
- Using
_prefix for intentionally unused parameters needed for fixture side-effects
As per coding guidelines: "Do not disable lint warnings with # noqa, # type: ignore, or # pylint: disable comments - fix the underlying issue instead"
🤖 Prompt for AI Agents
In `@tests/unit/runner/test_evaluation.py` around lines 1 - 2, Remove the
file-level "# pylint: disable=unused-argument" from
tests/unit/runner/test_evaluation.py and fix the underlying unused-argument
issues by either deleting parameters that are not used in the test functions or
renaming intentionally unused fixture parameters to start with an underscore
(e.g., change "fixture" to "_fixture") so they remain for side-effects; scan all
test functions in test_evaluation.py for unused parameters and apply the same
pattern (match what was done in test_processor.py) to satisfy lint rules without
suppressions.
2 participants
Description
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.