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
load_text only creates populations if none given #1910
Conversation
5117633
to
c739b4b
Compare
Looks good. It would be nice to add a couple of minimal tests like (paraphrased) def test_parse_text_no_populations(self):
nodes = """\
is_sample time population
1 0 2
"""
tables = parse_text(nodes)
assert len(tables.nodes) == 1
assert len(tables.populations) == 3
def test_parse_text_populations(self):
populations = """\
metadata
metadata1
metadata2
"""
tables = parse_text(populations)
assert len(tables.populations) == 2
assert tables.popualtions[0].metadata == "metadata1"
assert tables.popualtions[0].metadata == "metadata2" This will exercise both paths. I'm becoming a big fan of small obvious tests, they're so much more effective than the big elaborate code-based approaches that I used to favour. |
Codecov Report
@@ Coverage Diff @@
## main #1910 +/- ##
=======================================
Coverage 93.16% 93.16%
=======================================
Files 27 27
Lines 25094 25094
Branches 1109 1109
=======================================
Hits 23379 23379
Misses 1681 1681
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Good call. This exposed another bug: we were forgetting to strip the "\n" from each line when doing strict text reading. I've added this in, but apart from the test you suggested above, which only applied to the population table, this change doesn't have an associated unit test. (I've also changed the |
daa0cf7
to
857649f
Compare
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
857649f
to
7f62573
Compare
Fixes #1909
PR Checklist: