Add several tests for different formatting methods #260
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #260 +/- ##
==========================================
+ Coverage 75.26% 79.92% +4.66%
==========================================
Files 41 41
Lines 4180 4180
==========================================
+ Hits 3146 3341 +195
+ Misses 1034 839 -195 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for working on upping our test game. I left some feedback on...
- reducing the number of parameters set in
@pytest.mark.parametrize(which were resulting in things having to be constantly reset back to defaults) - leaning away from using the same long list of inputs across many case, in favor of individual inputs which test new & distinct bits of underlying logic
- providing more informative assertions for fmt_nanoplot, and using fewer inputs (or clarifying what we're testing their above and beyond what's tested for
_generate_nanoplot).
tests/test_formats.py
Outdated
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "time_style,x_out", |
There was a problem hiding this comment.
Since each case is run against the same 7 input times, it's not clear which of these inputs tests something new per timestyle, and which is redundant.
Rather than each case involving a time_style and a the same list of 7 inputs, can you make each case a time_style, 1 input, and the output expected? A case should only be added if it tests some new bit underlying logic.
tests/test_formats.py
Outdated
| # ------------------------------------------------------------------------------ | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Same feedback as above, on splitting into single input cases. It appears the underlying logic is largely babel's format_datetime, so you don't need to re-test that (but it makes sense there's a lot of stitching of arguments to fmt_datetime that need to get tested).
tests/test_formats.py
Outdated
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "standard,decimals,use_seps,force_sign,incl_space,x_out", |
There was a problem hiding this comment.
See feedback on avoiding using many parameters, and resetting things to their default arguments.
See also feedback on using 1 long input across all cases. I can't tell which values in the output tell us something new. For example all the outputs contain "-342 B", so it's not clear what I should learn there across the 5 parameter set cases.
(Sometimes tests do need to validate that some result is unchanged, but it seems likely the testing here is maybe doing that repeatedly for the same underlying logic)
tests/test_formats.py
Outdated
| ), | ||
| ], | ||
| ) | ||
| def test_fmt_bytes( |
There was a problem hiding this comment.
See feedback on parametrizing and not using the same inputs for everything
tests/test_formats.py
Outdated
| # ------------------------------------------------------------------------------ | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
I strongly suspect you don't need to test that upper works on many variations of inputs values. (one is probably fine).
tests/test_formats.py
Outdated
| return all(bool(re.match("^<div><svg.*</svg></div>$", x)) for x in nanoplot_str) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
It looks like many of the tests below...
- test across a wide range of inputs (10 rows in a DataFrame)
- only assert that the nanoplot exists with a div and svg tag
Please re-write so that it clear what each cases is testing, and their is a more specific assertion. For example, if reference line is "mean", does that mean the line is the average across rows? If so, in order to write these tests, you may need to reduce the number of input rows (e.g. to two), and test the exact value of the reference line in resulting nanoplots.
There was a problem hiding this comment.
If some of these parameters end up being tested more thoroughly in another place (e.g. _generate_nanoplots), then we should think about what parameters need to be tested at this level and how thoroughly (based on the underlying logic in the fmt_nanoplot function).
tests/test_formats.py
Outdated
| assert _all_vals_nanoplot_output(res) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
See above feedback. It's not clear what specific behaviors are being tested here. There are likely too many inputs, and the assertions are not specific enough.
It's okay to include the inputs in the parameters, if each case requires it's own (minimal) set of inputs to be meaningful. If the specification of arguments, inputs, and results checks are two cumbersome for @pytest.mark.parametrize, then it may be a signal to put each in its own function.
tests/test_formats.py
Outdated
| assert _all_vals_nanoplot_output(res_2) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
|
I believe all the comments have been addressed through the changes we made during code pairing. I handled a merge conflict and reformatted to satisfy the pre-commit error. |
Quite a few of the formatting methods had very low test coverage so this PR provides some testing for the following:
fmt_roman()fmt_bytes()fmt_time()fmt_datetime()fmt_markdown()fmt_nanoplot()