-
Notifications
You must be signed in to change notification settings - Fork 22
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
"acceptance" tests #305
"acceptance" tests #305
Conversation
Codecov Report
@@ Coverage Diff @@
## main #305 +/- ##
==========================================
+ Coverage 91.54% 91.63% +0.08%
==========================================
Files 41 43 +2
Lines 2260 2318 +58
==========================================
+ Hits 2069 2124 +55
- Misses 191 194 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look great. Two minor suggestions to consider before merging.
@@ -65,7 +65,8 @@ plugins = "numpy.typing.mypy_plugin" | |||
|
|||
[tool.pytest.ini_options] | |||
markers = [ | |||
"live_corpus: runs on the live Cell Census data corpus", | |||
"live_corpus: runs on the live Cell Census data corpus and small enough to run in CI", | |||
"expensive: too expensive to run regularly or in CI", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
These tests are periodically run, and are not part of CI due to their overhead. | ||
|
||
Please record: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please record: | |
When run, please record the results below and commit to git: |
@pytest.mark.live_corpus | ||
@pytest.mark.parametrize("organism", ["homo_sapiens", "mus_musculus"]) | ||
@pytest.mark.parametrize( | ||
"obs_value_filter", ["tissue=='aorta'", pytest.param("tissue=='brain'", marks=pytest.mark.expensive)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pytest is great; TIL: param values can add marks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always pays to RT*M :-)
- date | ||
- host / instance type | ||
- Python & package versions and OS (tip: use tiledbsoma.show_package_versions()) | ||
- full output of: `pytest --durations=0 --expensive ./api/python/cell_census/tests/` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, the cell census release, unless that shows up in the output? motivation is to know what data size tests last passed for. Maybe each test could output relevant sizes on full reads, like obs. Or output len(obs), len(var), X.nnz once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes on recording the version aliased to latest
I'm not sure we want the tests generating a bunch of output unless it is regularly used. I think your census version is the ideal solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #185
Notes to reviewer: