Skip to content

refactor: remove assert statement from non-test files #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
deepsource-autofix wants to merge 1 commit into from
+74 −37
Closed

refactor: remove assert statement from non-test files #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions scripts/ci/model_calibration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ def test_model_calibration():
# Test temperature setting
logger.info("Testing temperature calibration...")
model.temperature.data.fill_(1.0)
assert model.temperature.item() == 1.0, "Temperature not set correctly"
if model.temperature.item() != 1.0:
raise AssertionError("Temperature not set correctly")
Comment on lines +115 to +116

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In test files, assert statements are preferred over if/raise AssertionError. Test frameworks like pytest use assert to provide detailed failure reports, including the values of variables in the asserted expression. This change removes that benefit, making test failures harder to debug.

assert model.temperature.item() == 1.0, "Temperature not set correctly"

logger.info("✅ Temperature calibration works")

# Create test data
Expand Down Expand Up @@ -156,8 +157,10 @@ def test_model_calibration():
logger.info(f"Macro F1: {macro_f1:.4f}")

# Basic validation
assert 0 <= micro_f1 <= 1, f"Invalid F1 score: {micro_f1}"
assert 0 <= macro_f1 <= 1, f"Invalid F1 score: {macro_f1}"
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}")
Comment on lines +160 to +163

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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}"


logger.info("✅ Metrics calculation works")

Expand Down
12 changes: 8 additions & 4 deletions scripts/ci/model_compression_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,12 @@ def test_model_compression():
logger.info(f"Inference speedup: {speedup:.2f}x")

# Basic validation
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 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}")
Comment on lines +136 to +141

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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}"


# Test model saving
logger.info("Testing model saving...")
Expand All @@ -152,7 +155,8 @@ def test_model_compression():
}, output_path)

# Verify file was created
assert output_path.exists(), f"Compressed model file not created: {output_path}"
if not output_path.exists():
raise AssertionError(f"Compressed model file not created: {output_path}")
Comment on lines +158 to +159

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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}"

logger.info(f"✅ Compressed model saved to {output_path}")

# Clean up
Expand Down
78 changes: 52 additions & 26 deletions scripts/ci/model_monitoring_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,15 @@ def test_performance_tracker():

# Test current performance
current_perf = tracker.get_current_performance()
assert "f1_score" in current_perf
assert current_perf["f1_score"] == 0.75
if "f1_score" not in current_perf:
raise AssertionError
if current_perf["f1_score"] != 0.75:
raise AssertionError
Comment on lines +74 to +77

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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


# Test trend analysis
trend = tracker.get_trend_analysis()
assert "insufficient_data" in trend
if "insufficient_data" not in trend:
raise AssertionError
Comment on lines +81 to +82

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Similar to the previous comment, removing the assert statement and raising a bare AssertionError makes test failures opaque. The original assert is self-documenting and provides better failure messages with test runners.

assert "insufficient_data" in trend


# Test degradation detection
degradation_alert = tracker.detect_degradation()
Expand Down Expand Up @@ -121,7 +124,8 @@ def test_drift_detector():

drift_metrics = detector.detect_drift(current_data)
assert isinstance(drift_metrics, DriftMetrics)
assert not drift_metrics.drift_detected
if drift_metrics.drift_detected:
raise AssertionError
Comment on lines +127 to +128

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This change is detrimental to test debugging as it raises an AssertionError without any context. The original assert not drift_metrics.drift_detected is much clearer. Furthermore, this change is inconsistent, as the assert isinstance(...) on the line above was not changed. All assertions in tests should use the assert keyword.

assert not drift_metrics.drift_detected


# Test with drifted data
drifted_data = pd.DataFrame(
Expand Down Expand Up @@ -161,10 +165,14 @@ def test_alert_system():
)

# Test alert properties
assert alert.alert_type == "TEST_ALERT"
assert alert.severity == "MEDIUM"
assert alert.message == "Test alert message"
assert not alert.action_required
if alert.alert_type != "TEST_ALERT":
raise AssertionError
if alert.severity != "MEDIUM":
raise AssertionError
if alert.message != "Test alert message":
raise AssertionError
if alert.action_required:
raise AssertionError
Comment on lines +168 to +175

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

Replacing this block of assert statements with if/raise is harmful to the test suite. Each of these new checks raises a bare AssertionError, hiding the cause of the failure. For example, if alert.severity is not 'MEDIUM', the test will fail without indicating what the actual value was.

assert alert.alert_type == "TEST_ALERT"
assert alert.severity == "MEDIUM"
assert alert.message == "Test alert message"
assert not alert.action_required


# Test alert serialization
alert_dict = {
Expand Down Expand Up @@ -201,7 +209,8 @@ def test_config_creation():

# Check if config file was created
config_path = Path(test_config_path)
assert config_path.exists()
if not config_path.exists():
raise AssertionError
Comment on lines +212 to +213

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This check should use assert for better test failure diagnostics. Raising a bare AssertionError provides no information about why the check failed.

assert config_path.exists()


# Load and validate config
import yaml
Expand All @@ -210,10 +219,14 @@ def test_config_creation():
config = yaml.safe_load(f)

# Check required fields
assert "model_path" in config
assert "window_size" in config
assert "monitor_interval" in config
assert "alert_threshold" in config
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
Comment on lines +222 to +229

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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


# Clean up
config_path.unlink()
Expand Down Expand Up @@ -254,15 +267,21 @@ def test_monitoring_initialization():
monitor = ModelHealthMonitor(test_config_path)

# Test basic properties
assert monitor.config["window_size"] == 10
assert monitor.config["alert_threshold"] == 0.1
assert not monitor.monitoring_active
if monitor.config["window_size"] != 10:
raise AssertionError
if monitor.config["alert_threshold"] != 0.1:
raise AssertionError
if monitor.monitoring_active:
raise AssertionError
Comment on lines +270 to +275

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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


# Test health status
health_status = monitor.get_health_status()
assert "timestamp" in health_status
assert "model_loaded" in health_status
assert "monitoring_active" in health_status
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
Comment on lines +279 to +284

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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


# Clean up
Path(test_config_path).unlink()
Expand Down Expand Up @@ -295,13 +314,20 @@ def test_metrics_collection():
)

# Test metrics properties
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 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
Comment on lines +317 to +330

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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


# Test metrics serialization
metrics_dict = {
Expand Down
12 changes: 8 additions & 4 deletions scripts/ci/t5_summarization_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,12 @@ def test_t5_model_loading():
logger.info("✅ T5 model initialized successfully")

# Test basic model properties
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 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")
Comment on lines +37 to +42

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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"


logger.info("✅ Model attributes validation passed")

Expand Down Expand Up @@ -71,7 +74,8 @@ def test_t5_summarization():

# Validate summary
assert isinstance(summary, str), "Summary should be a string"
assert len(summary) > 0, "Summary should not be empty"
if len(summary) <= 0:
raise AssertionError("Summary should not be empty")
Comment on lines +77 to +78

@gemini-code-assist gemini-code-assist Bot Jul 29, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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"

assert len(summary) < len(test_text), "Summary should be shorter than input"

logger.info("✅ Summary validation passed")
Expand Down