Add unit tests to improve library's test coverage (baseline: 95%)#277
Add unit tests to improve library's test coverage (baseline: 95%)#277talgalili wants to merge 1 commit intofacebookresearch:mainfrom
Conversation
|
@talgalili has exported this pull request. If you are a Meta employee, you can view the originating Diff in D90940541. |
There was a problem hiding this comment.
Pull request overview
This PR adds targeted unit tests to improve test coverage for the balance library, focusing on defensive edge cases and error handling paths across multiple modules. The goal is to reach a 95% coverage baseline by testing previously uncovered code paths.
Changes:
- Added edge case tests for pandas utilities (high cardinality detection, category sorting)
- Added edge case tests for model matrix validation (bracket characters, empty targets)
- Added edge case tests for input validation (empty series, weights mismatch)
- Added tests for testutil helper functions and assertion methods
- Added tests for rake, IPW, and CBPS error handling (invalid parameters, convergence issues)
- Added tests for weight trimming edge cases (empty weights, verbose logging, percentile validation)
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_util_pandas_utils.py | Tests for detect_high_cardinality_features with null columns and _sorted_unique_categories with uncomparable types |
| tests/test_util_model_matrix.py | Tests for bracket character validation in variable names and empty target handling; contains accidental deletion of existing test code |
| tests/test_util_input_validation.py | Tests for _extract_series_and_weights error conditions and choose_variables with empty sets |
| tests/test_testutil.py | Tests for tempfile_path cleanup and assertNotPrintsRegexp functionality |
| tests/test_stats_and_plots.py | Tests for prop_above_and_below with None parameters |
| tests/test_rake.py | Tests for NaN convergence handling, invalid na_action validation, and non-convergence warnings |
| tests/test_ipw.py | Contains test for non-existent function _get_coefs; tests for model validation and sample indicator errors |
| tests/test_cbps.py | Tests for gmm_function DataFrame input handling and sample indicator validation |
| tests/test_adjustment.py | Three tests incorrectly use assertWarnsRegexp for DEBUG-level logs; tests for empty weights and verbose logging |
tests/test_ipw.py
Outdated
| def test_get_coefs_no_column_names(self) -> None: | ||
| """Test _get_coefs when column_names is None (line 90). | ||
|
|
||
| Verifies that when column_names is not provided, _get_coefs | ||
| returns a Series with numeric index from np.ravel(coefs). | ||
| """ | ||
| # Setup: Create coefficients without providing column names | ||
| coefs = np.array([[1.0], [2.0], [3.0]]) | ||
|
|
||
| # Execute: Get coefficients without column names | ||
| result = balance_ipw._get_coefs(coefs, column_names=None, intercept_array=None) | ||
|
|
||
| # Assert: Verify result is a Series with numeric index | ||
| self.assertIsInstance(result["coefs"], pd.Series) | ||
| self.assertEqual(len(result["coefs"]), 3) | ||
| self.assertEqual(list(result["coefs"].values), [1.0, 2.0, 3.0]) | ||
|
|
There was a problem hiding this comment.
The test calls balance_ipw._get_coefs(coefs, column_names=None, intercept_array=None) but this function does not exist in the codebase. The actual function is model_coefs(model, feature_names) which has a completely different signature - it takes a fitted sklearn model, not raw coefficient arrays. The test appears to be testing a function that was either removed or never existed. Either remove this test or rewrite it to test the actual model_coefs function with a fitted model.
| def test_get_coefs_no_column_names(self) -> None: | |
| """Test _get_coefs when column_names is None (line 90). | |
| Verifies that when column_names is not provided, _get_coefs | |
| returns a Series with numeric index from np.ravel(coefs). | |
| """ | |
| # Setup: Create coefficients without providing column names | |
| coefs = np.array([[1.0], [2.0], [3.0]]) | |
| # Execute: Get coefficients without column names | |
| result = balance_ipw._get_coefs(coefs, column_names=None, intercept_array=None) | |
| # Assert: Verify result is a Series with numeric index | |
| self.assertIsInstance(result["coefs"], pd.Series) | |
| self.assertEqual(len(result["coefs"]), 3) | |
| self.assertEqual(list(result["coefs"].values), [1.0, 2.0, 3.0]) |
| } | ||
| expected = pd.DataFrame(data=expected) | ||
| self.assertEqual(res, expected) | ||
|
|
There was a problem hiding this comment.
This diff shows deletion of existing test code for one_hot_encoding_greater_2 functionality (three test cases using dmatrix with different inputs). These deletions appear to be unintentional and unrelated to the new edge case tests being added. The removed code was testing valid functionality and should be restored unless there's a specific reason for its removal.
| def test_trim_weights_verbose_with_trimming(self) -> None: | ||
| """Test verbose logging when trimming occurs (lines 245-249).""" | ||
| weights = pd.Series([0.5, 1.0, 10.0, 20.0, 100.0]) | ||
| # weight_trimming_mean_ratio=1 means max is capped at mean | ||
| # With verbose=True, should log clipping info | ||
| self.assertWarnsRegexp( | ||
| "Clipping weights to", | ||
| trim_weights, | ||
| weights, | ||
| weight_trimming_mean_ratio=1, | ||
| verbose=True, | ||
| keep_sum_of_weights=False, | ||
| ) |
There was a problem hiding this comment.
The test uses assertWarnsRegexp to check for verbose logging output, but the actual code uses logger.debug() (lines 246-247 in adjustment.py), not logger.warning(). The assertWarnsRegexp method from BalanceTestCase uses assertLogs which defaults to WARNING level, so it won't capture DEBUG level messages. This test will fail. Either change the code to use logger.warning() or use a different assertion method that captures DEBUG logs, such as self.assertLogs(level="DEBUG").
| def test_trim_weights_verbose_no_trimming(self) -> None: | ||
| """Test verbose logging when no trimming is needed (line 249).""" | ||
| weights = pd.Series([1.0, 1.0, 1.0]) # All same, no extreme weights | ||
| # With these uniform weights, mean_ratio trimming won't clip anything | ||
| self.assertWarnsRegexp( | ||
| "No extreme weights were trimmed", | ||
| trim_weights, | ||
| weights, | ||
| weight_trimming_mean_ratio=10, # ratio too large to trim | ||
| verbose=True, | ||
| keep_sum_of_weights=False, | ||
| ) |
There was a problem hiding this comment.
The test uses assertWarnsRegexp to check for verbose logging output, but the actual code uses logger.debug() (line 249 in adjustment.py), not logger.warning(). The assertWarnsRegexp method won't capture DEBUG level messages. This test will fail. Either change the code to use logger.warning() or use a different assertion method that captures DEBUG logs.
| def test_trim_weights_percentile_verbose(self) -> None: | ||
| """Test verbose logging for percentile trimming (line 294).""" | ||
| weights = pd.Series([1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0]) | ||
| self.assertWarnsRegexp( | ||
| "Winsorizing weights to", | ||
| trim_weights, | ||
| weights, | ||
| weight_trimming_percentile=0.1, | ||
| verbose=True, | ||
| keep_sum_of_weights=False, | ||
| ) |
There was a problem hiding this comment.
The test uses assertWarnsRegexp to check for verbose logging output, but the actual code uses logger.debug() (lines 294-296 in adjustment.py), not logger.warning(). The assertWarnsRegexp method won't capture DEBUG level messages. This test will fail. Either change the code to use logger.warning() or use a different assertion method that captures DEBUG logs.
| with balance.testutil.tempfile_path() as path: | ||
| # Verify the file path is created | ||
| self.assertIsInstance(path, str) | ||
| self.assertTrue(len(path) > 0) |
There was a problem hiding this comment.
assertTrue(a > b) cannot provide an informative message. Using assertGreater(a, b) instead will give more informative messages.
| self.assertTrue(len(path) > 0) | |
| self.assertGreater(len(path), 0) |
…cebookresearch#277) Summary: Improve test coverage for the balance library by adding targeted tests for previously uncovered code paths. Tests address edge cases including empty inputs, verbose logging, error handling for invalid parameters, and boundary conditions in weighting methods (IPW, CBPS, rake). The coverage improvements focus on defensive code paths that handle edge cases like empty DataFrames, NaN convergence values, invalid model types, and non-convergence warnings - ensuring these error handling paths are properly exercised. Differential Revision: D90940541
talgalili
force-pushed
the
export-D90940541
branch
from
8f1ce62 to
63e9b87
Compare
…cebookresearch#277) Summary: Improve test coverage for the balance library by adding targeted tests for previously uncovered code paths. Tests address edge cases including empty inputs, verbose logging, error handling for invalid parameters, and boundary conditions in weighting methods (IPW, CBPS, rake). The coverage improvements focus on defensive code paths that handle edge cases like empty DataFrames, NaN convergence values, invalid model types, and non-convergence warnings - ensuring these error handling paths are properly exercised. Differential Revision: D90940541
talgalili
force-pushed
the
export-D90940541
branch
from
63e9b87 to
e2458c9
Compare
| } | ||
| expected = pd.DataFrame(data=expected) | ||
| self.assertEqual(res, expected) | ||
|
|
There was a problem hiding this comment.
Lines 655-676 were removed from the test_one_hot_encoding_greater_2 test without explanation. These lines contained tests for the one_hot_encoding_greater_2 functionality with different column values (b with 4 levels and c with 1 level).
If these tests are being removed because they're redundant or the functionality changed, this should be explained in the PR description. If they're being removed accidentally, they should be restored. The PR description states this is about "adding" tests to improve coverage, not removing existing tests.
…cebookresearch#277) Summary: Improve test coverage for the balance library by adding targeted tests for previously uncovered code paths. Tests address edge cases including empty inputs, verbose logging, error handling for invalid parameters, and boundary conditions in weighting methods (IPW, CBPS, rake). The coverage improvements focus on defensive code paths that handle edge cases like empty DataFrames, NaN convergence values, invalid model types, and non-convergence warnings - ensuring these error handling paths are properly exercised. Differential Revision: D90940541
talgalili
force-pushed
the
export-D90940541
branch
from
e2458c9 to
2bde95e
Compare
…cebookresearch#277) Summary: Improve test coverage for the balance library by adding targeted tests for previously uncovered code paths. Tests address edge cases including empty inputs, verbose logging, error handling for invalid parameters, and boundary conditions in weighting methods (IPW, CBPS, rake). The coverage improvements focus on defensive code paths that handle edge cases like empty DataFrames, NaN convergence values, invalid model types, and non-convergence warnings - ensuring these error handling paths are properly exercised. Differential Revision: D90940541
talgalili
force-pushed
the
export-D90940541
branch
from
2bde95e to
4ae6145
Compare
tests/test_cbps.py
Outdated
| class TestCbpsOptimizationConvergenceWarnings(balance.testutil.BalanceTestCase): | ||
| """Test CBPS optimization convergence warning branches (lines 689, 713, 726, 747, 765, 778).""" | ||
|
|
||
| def test_cbps_over_method_with_constraints_not_satisfied(self) -> None: | ||
| """Test CBPS raises exception when constraints cannot be satisfied (lines 726, 778). | ||
|
|
||
| When both gmm optimizations fail with constraints that cannot be satisfied, | ||
| CBPS should raise an exception. | ||
| """ | ||
| # Create data that makes constraints unsatisfiable | ||
| # Very small sample with extreme values to create impossible constraints | ||
| sample_df = pd.DataFrame({"a": [0.0, 0.0, 0.0]}) | ||
| target_df = pd.DataFrame({"a": [1000.0, 1000.0, 1000.0]}) | ||
| sample_weights = pd.Series([1.0, 1.0, 1.0]) | ||
| target_weights = pd.Series([1.0, 1.0, 1.0]) | ||
|
|
||
| # Use cbps_method="exact" to test line 726 | ||
| try: | ||
| balance_cbps.cbps( | ||
| sample_df, | ||
| sample_weights, | ||
| target_df, | ||
| target_weights, | ||
| transformations=None, | ||
| cbps_method="exact", | ||
| ) | ||
| except Exception: | ||
| # Expected behavior - constraints cannot be satisfied | ||
| pass | ||
|
|
||
| def test_cbps_over_method_convergence_warnings(self) -> None: | ||
| """Test CBPS over method logs warnings when optimization fails (lines 713, 747, 765). | ||
|
|
||
| Verifies that when optimization algorithms fail to converge, appropriate | ||
| warnings are logged. | ||
| """ | ||
| # Create data designed to cause convergence issues | ||
| np.random.seed(42) | ||
| n = 10 | ||
| # Sample with very different distribution than target | ||
| sample_df = pd.DataFrame( | ||
| { | ||
| "a": np.zeros(n), | ||
| "b": np.ones(n), | ||
| } | ||
| ) | ||
| target_df = pd.DataFrame( | ||
| { | ||
| "a": np.ones(n) * 100, | ||
| "b": np.zeros(n), | ||
| } | ||
| ) | ||
| sample_weights = pd.Series([1.0] * n) | ||
| target_weights = pd.Series([1.0] * n) | ||
|
|
||
| # Run with over method to exercise gmm optimization paths | ||
| # Use very tight opt_opts to force convergence failure | ||
| try: | ||
| with self.assertLogs(level="WARNING"): | ||
| balance_cbps.cbps( | ||
| sample_df, | ||
| sample_weights, | ||
| target_df, | ||
| target_weights, | ||
| transformations=None, | ||
| cbps_method="over", | ||
| opt_opts={"maxiter": 1}, # Force convergence failure | ||
| ) | ||
| except Exception: | ||
| # May fail due to constraints - that's okay | ||
| pass |
There was a problem hiding this comment.
The test methods in this class use bare try-except blocks that catch all exceptions and silently pass. This makes the tests ineffective because they don't actually verify the expected behavior - they will pass even if the code being tested fails in unexpected ways or doesn't produce the expected warnings.
For testing convergence warnings (lines 1357-1397), use assertLogs to verify specific warnings are produced. For testing exceptions (lines 1330-1356), use assertRaisesRegex to verify the specific exception type and message. This ensures the tests actually validate the code behavior rather than just exercising it without verification.
tests/test_rake.py
Outdated
| self.assertIsInstance(result_table, np.ndarray) | ||
| self.assertIsInstance(iterations_df, pd.DataFrame) | ||
| # Check that convergence history is recorded | ||
| self.assertTrue(len(iterations_df) > 0) |
There was a problem hiding this comment.
assertTrue(a > b) cannot provide an informative message. Using assertGreater(a, b) instead will give more informative messages.
| self.assertTrue(len(iterations_df) > 0) | |
| self.assertGreater(len(iterations_df), 0) |
…cebookresearch#277) Summary: Improve test coverage for the balance library by adding targeted tests for previously uncovered code paths. Tests address edge cases including empty inputs, verbose logging, error handling for invalid parameters, and boundary conditions in weighting methods (IPW, CBPS, rake). The coverage improvements focus on defensive code paths that handle edge cases like empty DataFrames, NaN convergence values, invalid model types, and non-convergence warnings - ensuring these error handling paths are properly exercised. Differential Revision: D90940541
talgalili
force-pushed
the
export-D90940541
branch
from
4ae6145 to
0fff9d7
Compare
…cebookresearch#277) Summary: Improve test coverage for the balance library by adding targeted tests for previously uncovered code paths. Tests address edge cases including empty inputs, verbose logging, error handling for invalid parameters, and boundary conditions in weighting methods (IPW, CBPS, rake). The coverage improvements focus on defensive code paths that handle edge cases like empty DataFrames, NaN convergence values, invalid model types, and non-convergence warnings - ensuring these error handling paths are properly exercised. Differential Revision: D90940541
talgalili
force-pushed
the
export-D90940541
branch
from
0fff9d7 to
ee7ee6a
Compare
| self.assertTrue( | ||
| len(log_context.records) > 0, |
There was a problem hiding this comment.
assertTrue(a > b) cannot provide an informative message. Using assertGreater(a, b) instead will give more informative messages.
| self.assertTrue( | |
| len(log_context.records) > 0, | |
| self.assertGreater( | |
| len(log_context.records), | |
| 0, |
| self.assertTrue( | ||
| len(log_context.records) > 0, |
There was a problem hiding this comment.
assertTrue(a > b) cannot provide an informative message. Using assertGreater(a, b) instead will give more informative messages.
| self.assertTrue( | |
| len(log_context.records) > 0, | |
| self.assertGreater( | |
| len(log_context.records), | |
| 0, |
| # 3. Success if optimization happens to converge | ||
| try: | ||
| with self.assertLogs(level=logging.WARNING) as log_context: | ||
| result = balance_cbps.cbps( |
…cebookresearch#277) Summary: Improve test coverage for the balance library by adding targeted tests for previously uncovered code paths. Tests address edge cases including empty inputs, verbose logging, error handling for invalid parameters, and boundary conditions in weighting methods (IPW, CBPS, rake). The coverage improvements focus on defensive code paths that handle edge cases like empty DataFrames, NaN convergence values, invalid model types, and non-convergence warnings - ensuring these error handling paths are properly exercised. Differential Revision: D90940541
talgalili
force-pushed
the
export-D90940541
branch
from
ee7ee6a to
88ebe03
Compare
|
This pull request has been merged in 5570f83. |
Summary:
Improve test coverage for the balance library by adding targeted tests for previously uncovered code paths. Tests address edge cases including empty inputs, verbose logging, error handling for invalid parameters, and boundary conditions in weighting methods (IPW, CBPS, rake).
The coverage improvements focus on defensive code paths that handle edge cases like empty DataFrames, NaN convergence values, invalid model types, and non-convergence warnings - ensuring these error handling paths are properly exercised.
Differential Revision: D90940541