refactor: remove assert statement from non-test files - #3
refactor: remove assert statement from non-test files#3deepsource-autofix[bot] wants to merge 1 commit into
Conversation
Usage of `assert` statement in application logic is discouraged. `assert` is removed with compiling to optimized byte code. Consider raising an exception instead. Ideally, `assert` statement should be used only in tests.
|
Here's the code health analysis summary for commits Analysis Summary
|
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors test files to replace assert statements with explicit AssertionError exceptions. The change addresses the issue that assert statements are removed when Python is compiled to optimized bytecode, which could cause tests to silently pass in production environments.
- Replaces
assertstatements with conditionalifstatements that raiseAssertionError - Maintains the same error messages for better debugging
- Ensures test assertions will execute even with Python optimization flags
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/ci/t5_summarization_test.py | Converts model attribute validation and summary length assertions to explicit exceptions |
| scripts/ci/model_monitoring_test.py | Refactors performance tracking, drift detection, and config validation assertions |
| scripts/ci/model_compression_test.py | Updates model size and performance validation assertions |
| scripts/ci/model_calibration_test.py | Converts temperature setting and F1 score validation assertions |
Comments suppressed due to low confidence (26)
scripts/ci/model_monitoring_test.py:75
- The AssertionError lacks a descriptive message. Consider adding a message like 'f1_score not found in current_perf' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:77
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected f1_score to be 0.75' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:82
- The AssertionError lacks a descriptive message. Consider adding a message like 'insufficient_data not found in trend' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:128
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected no drift to be detected' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:169
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected alert_type to be TEST_ALERT' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:171
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected severity to be MEDIUM' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:173
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected message to be Test alert message' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:175
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected action_required to be False' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:213
- The AssertionError lacks a descriptive message. Consider adding a message like 'Config file was not created' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:223
- The AssertionError lacks a descriptive message. Consider adding a message like 'model_path not found in config' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:225
- The AssertionError lacks a descriptive message. Consider adding a message like 'window_size not found in config' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:227
- The AssertionError lacks a descriptive message. Consider adding a message like 'monitor_interval not found in config' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:229
- The AssertionError lacks a descriptive message. Consider adding a message like 'alert_threshold not found in config' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:271
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected window_size to be 10' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:273
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected alert_threshold to be 0.1' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:275
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected monitoring_active to be False' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:280
- The AssertionError lacks a descriptive message. Consider adding a message like 'timestamp not found in health_status' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:282
- The AssertionError lacks a descriptive message. Consider adding a message like 'model_loaded not found in health_status' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:284
- The AssertionError lacks a descriptive message. Consider adding a message like 'monitoring_active not found in health_status' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:318
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected f1_score to be 0.75' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:320
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected precision to be 0.78' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:322
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected recall to be 0.72' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:324
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected inference_time_ms to be 150.0' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:326
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected throughput_rps to be 33.3' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:328
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected memory_usage_mb to be 512.0' to help with debugging.
raise AssertionError
scripts/ci/model_monitoring_test.py:330
- The AssertionError lacks a descriptive message. Consider adding a message like 'Expected gpu_utilization to be 45.0' to help with debugging.
raise AssertionError
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request aims to remove assert statements from application logic, which is a good practice. However, the changes are applied to test files, where assert statements are the standard and idiomatic way to write assertions in Python testing. Replacing assert with if/raise AssertionError in test files degrades test quality and debuggability. It is recommended to revert the changes in the test files and keep the assert statements. If the goal is to remove assert from non-test code, there is an assert statement in src/models/summarization/t5_summarizer.py on line 86 which would be a good candidate for this refactoring.
| if "model_path" not in config: | ||
| raise AssertionError | ||
| if "window_size" not in config: | ||
| raise AssertionError | ||
| if "monitor_interval" not in config: | ||
| raise AssertionError | ||
| if "alert_threshold" not in config: | ||
| raise AssertionError |
There was a problem hiding this comment.
This block of checks is another example where replacing assert with if/raise significantly reduces test clarity on failure. The original assert "model_path" in config is superior because a test runner can show the contents of config if the key is missing.
assert "model_path" not in config
assert "window_size" not in config
assert "monitor_interval" not in config
assert "alert_threshold" not in config| if monitor.config["window_size"] != 10: | ||
| raise AssertionError | ||
| if monitor.config["alert_threshold"] != 0.1: | ||
| raise AssertionError | ||
| if monitor.monitoring_active: | ||
| raise AssertionError |
There was a problem hiding this comment.
These property checks are part of a test and should use assert. Replacing them with if/raise AssertionError removes valuable debugging information that test runners provide on assert failures.
assert monitor.config["window_size"] == 10
assert monitor.config["alert_threshold"] == 0.1
assert not monitor.monitoring_active| if metrics.f1_score != 0.75: | ||
| raise AssertionError | ||
| if metrics.precision != 0.78: | ||
| raise AssertionError | ||
| if metrics.recall != 0.72: | ||
| raise AssertionError | ||
| if metrics.inference_time_ms != 150.0: | ||
| raise AssertionError | ||
| if metrics.throughput_rps != 33.3: | ||
| raise AssertionError | ||
| if metrics.memory_usage_mb != 512.0: | ||
| raise AssertionError | ||
| if metrics.gpu_utilization != 45.0: | ||
| raise AssertionError |
There was a problem hiding this comment.
This large block of if/raise statements is not suitable for a test. It makes debugging failures extremely difficult due to the lack of context in the raised exceptions. Please revert to using assert statements for all these checks to leverage the rich failure reporting of test frameworks.
assert metrics.f1_score == 0.75
assert metrics.precision == 0.78
assert metrics.recall == 0.72
assert metrics.inference_time_ms == 150.0
assert metrics.throughput_rps == 33.3
assert metrics.memory_usage_mb == 512.0
assert metrics.gpu_utilization == 45.0| if "timestamp" not in health_status: | ||
| raise AssertionError | ||
| if "model_loaded" not in health_status: | ||
| raise AssertionError | ||
| if "monitoring_active" not in health_status: | ||
| raise AssertionError |
There was a problem hiding this comment.
These health status checks should use assert to provide context on failure. For example, if 'timestamp' is not in health_status, pytest would show the content of health_status, which is lost with a bare raise AssertionError.
assert "timestamp" in health_status
assert "model_loaded" in health_status
assert "monitoring_active" in health_status| if "f1_score" not in current_perf: | ||
| raise AssertionError | ||
| if current_perf["f1_score"] != 0.75: | ||
| raise AssertionError |
There was a problem hiding this comment.
This change significantly degrades the test's utility. By replacing assert with raise AssertionError without a message, crucial context is lost on failure. The original assert statements would allow pytest to show the contents of current_perf or the value of current_perf['f1_score'], making debugging much easier.
assert "f1_score" in current_perf
assert current_perf["f1_score"] == 0.75| if not 0 <= micro_f1 <= 1: | ||
| raise AssertionError(f"Invalid F1 score: {micro_f1}") | ||
| if not 0 <= macro_f1 <= 1: | ||
| raise AssertionError(f"Invalid F1 score: {macro_f1}") |
There was a problem hiding this comment.
These checks should use assert as they are inside a test function. The assert 0 <= micro_f1 <= 1 syntax is not only concise but also allows test runners to provide more informative output on failure by displaying the value of micro_f1. The if/raise pattern is less suitable for tests.
assert 0 <= micro_f1 <= 1, f"Invalid F1 score: {micro_f1}"
assert 0 <= macro_f1 <= 1, f"Invalid F1 score: {macro_f1}"| if not output_path.exists(): | ||
| raise AssertionError(f"Compressed model file not created: {output_path}") |
There was a problem hiding this comment.
This check for file existence is part of a test and should use assert. Using assert is idiomatic in tests and allows test frameworks to provide better diagnostics.
assert output_path.exists(), f"Compressed model file not created: {output_path}"| if quantized_size >= original_size: | ||
| raise AssertionError(f"Quantized model should be smaller: {quantized_size} >= {original_size}") | ||
| if size_reduction <= 0: | ||
| raise AssertionError(f"Size reduction should be positive: {size_reduction}") | ||
| if speedup <= 0: | ||
| raise AssertionError(f"Speedup should be positive: {speedup}") |
There was a problem hiding this comment.
As this is a test file, these validations should be written using assert. This is the standard convention for Python tests and provides better integration with testing frameworks, leading to more descriptive failure messages. The current implementation makes debugging harder.
assert quantized_size < original_size, f"Quantized model should be smaller: {quantized_size} >= {original_size}"
assert size_reduction > 0, f"Size reduction should be positive: {size_reduction}"
assert speedup > 0, f"Speedup should be positive: {speedup}"| if not hasattr(model, "model"): | ||
| raise AssertionError("Model should have 'model' attribute") | ||
| if not hasattr(model, "tokenizer"): | ||
| raise AssertionError("Model should have 'tokenizer' attribute") | ||
| if not hasattr(model, "device"): | ||
| raise AssertionError("Model should have 'device' attribute") |
There was a problem hiding this comment.
These checks for model attributes are in a test function and should use assert. The assert statement is the standard for tests in Python and provides better failure diagnostics with tools like pytest.
assert hasattr(model, "model"), "Model should have 'model' attribute"
assert hasattr(model, "tokenizer"), "Model should have 'tokenizer' attribute"
assert hasattr(model, "device"), "Model should have 'device' attribute"| if len(summary) <= 0: | ||
| raise AssertionError("Summary should not be empty") |
There was a problem hiding this comment.
This change should be reverted. assert is the correct tool for this check within a test. Also, this change is inconsistent with the surrounding assert statements on lines 76 and 79, which were not modified. For consistency and better test reporting, all assertions in this block should use assert.
assert len(summary) > 0, "Summary should not be empty"
Usage of
assertstatement in application logic is discouraged.assertis removed with compiling to optimized byte code. Consider raising an exception instead. Ideally,assertstatement should be used only in tests.