Skip to content
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.

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

add unit tests for python cell_census package #244

Merged
merged 6 commits into from
Mar 6, 2023

Conversation

bkmartinjr
Copy link
Contributor

@bkmartinjr bkmartinjr commented Mar 6, 2023

Changes - more/better unit tests for the cell_census python package:

  • add error condition unit test coverage for open_soma and other API
  • add tests to cover all open_soma parameters
  • add missing absolute URI test for uri_join

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #244 (e2c80d3) into main (d0927af) will increase coverage by 1.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #244      +/-   ##
==========================================
+ Coverage   83.92%   85.07%   +1.15%     
==========================================
  Files          29       29              
  Lines        1642     1682      +40     
==========================================
+ Hits         1378     1431      +53     
+ Misses        264      251      -13     
Flag Coverage Δ
unittests 85.07% <100.00%> (+1.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/python/cell_census/tests/test_directory.py 100.00% <100.00%> (ø)
api/python/cell_census/tests/test_get_helpers.py 100.00% <100.00%> (ø)
api/python/cell_census/tests/test_open.py 100.00% <100.00%> (ø)
api/python/cell_census/tests/test_util.py 100.00% <100.00%> (ø)
.../cell_census/src/cell_census/_release_directory.py 100.00% <0.00%> (+10.00%) ⬆️
api/python/cell_census/src/cell_census/_util.py 100.00% <0.00%> (+11.11%) ⬆️
api/python/cell_census/src/cell_census/_open.py 100.00% <0.00%> (+18.00%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bkmartinjr bkmartinjr marked this pull request as ready for review March 6, 2023 21:55
@bkmartinjr bkmartinjr changed the title add unit tests add unit tests for python cell_census package Mar 6, 2023
Copy link
Member

@ebezzi ebezzi left a comment

Choose a reason for hiding this comment

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

LGTM, one nit and one more generic comment (not strictly related to this PR).


@pytest.mark.live_corpus
def test_download_source_h5ad(tmp_path: pathlib.Path, small_dataset_id: str) -> None:
# with cell_census.open_soma(census_version="latest") as census:
Copy link
Member

Choose a reason for hiding this comment

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

Can the commented code be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops! Yes, definitely.

@@ -60,6 +63,11 @@ def test_get_census_version_directory(directory_mock: Any) -> None:
assert directory[tag] == cell_census.get_census_version_description(tag)


def test_get_census_version_description_errors() -> None:
with pytest.raises(KeyError):
Copy link
Member

Choose a reason for hiding this comment

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

Not strictly related to the tests, but I was wondering if maybe we don't want to specialize these exceptions? Right now everything is KeyError or ValueError but these exceptions map to very specific issues (for instance, non existent census version.

Copy link
Contributor Author

@bkmartinjr bkmartinjr Mar 6, 2023

Choose a reason for hiding this comment

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

sure, we could do that.

I agree it is a separate issue and I'll file it.

See #245

@bkmartinjr bkmartinjr merged commit 655809c into main Mar 6, 2023
@bkmartinjr bkmartinjr deleted the bkmartinjr/unit-test-cov branch March 6, 2023 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants