pp test expansion#232
Conversation
mschwoer
left a comment
mschwoer
left a comment
There was a problem hiding this comment.
some general comments..
maybe have a look at https://github.com/MannLabs/claude_menagerie/tree/main/Magnus/skills/ms-python-unit-test-creator
| # from the public API. Kept as a documented "None means match-all" guard. | ||
| if v is None: | ||
| current_mask = pd.Series(True, index=data.index) # noqa: FBT003 | ||
| current_mask = pd.Series(True, index=data.index) # noqa: FBT003 # pragma: no cover |
There was a problem hiding this comment.
why no cover by tests? (for the above raise) statements it might be fine to not cover them, this is a functional piece of logic)
| apt.pp.add_metadata(adata, md, axis=0) | ||
|
|
||
|
|
||
| def test_add_metadata_verbose_log(example_data, caplog): |
There was a problem hiding this comment.
do we gain anything from testing logging?
unless it is super critical, I would remove it (adds to total cost)
There was a problem hiding this comment.
do you mean removing the logging altogether or # pragma: no cover the branch?
There was a problem hiding this comment.
I mean removing the test, with an optional pragma
| assert "NA" in result.obs["batch"].tolist() | ||
| assert not result.obs["batch"].isna().any() |
There was a problem hiding this comment.
could we test for the full result here?
| result = data_index_to_array(data, dim_space=dim_space) | ||
|
|
||
| # then | ||
| assert isinstance(result, np.ndarray) |
There was a problem hiding this comment.
test for full result.. isinstance is very weak (also for some other tests further down)
| assert np.array_equal(result, expected) | ||
|
|
||
|
|
||
| def test_data_index_to_array_invalid_dim_space(example_data): |
There was a problem hiding this comment.
docstrings? they really help getting the intent (more than just the name)
| @pytest.mark.parametrize( | ||
| ("obj", "expected"), | ||
| [ | ||
| ("foo", ["foo"]), |
There was a problem hiding this comment.
add: empty list?
("foo", "bar", ["fuh", "baz",]), ?
| assert isinstance(result, pd.DataFrame) | ||
| assert list(result.columns) == ["G1", "G2", "G3"] |
There was a problem hiding this comment.
again: quite weak asserts
2 participants
Purpose of this PR
Test coverage of
ppis de facto complete for all critical parts, the only thing missing was tests for two helpersdata_index_to_array&subset_dataand some tests for error-branches. This PR adds these missing components and raises thepptest coverage to 100 %.