augur icon indicating copy to clipboard operation
augur copied to clipboard

filter: Update functional test coverage

Open victorlin opened this issue 2 years ago • 2 comments

Description of proposed changes

Improves coverage of existing functional tests, motivated by work in #854. See individual commits.

Related issue(s)

N/A

Testing

See tests.

victorlin avatar May 13 '22 21:05 victorlin

@huddlej

In this PR, I'm worried that modifying the sequence index directly and copying the metadata TSV to CSV will decouple the original source data from the test outputs.

Valid, I made these changes naively with the single goal of increasing code coverage. I hadn't thought about decoupling.

Regarding the metadata CSV input, this feels like more of a test of io.read_metadata than augur filter. Is there CSV-specific filter logic in the sqlite branch that needs testing there? Or can we drop the metadata CSV test here?

I agree that this is more a test of io.read_metadata and should be moved. Mainly since we support CSV input, I wanted to test that this functionality also makes its way to the sqlite engine. It's currently not using io.read_metadata, which is another problem I'll need to solve first. But overall, more coverage in filter and supporting modules like io makes me more confident that the rewrite has functional parity with the existing pandas engine.

Maybe we can have a separate conversation about how to manage data in functional tests? Even if the answer is to just document the plan I had in my head, that would be better than nothing... :)

Good idea, maybe a README alongside data files will suffice. I'll start a PR based on your 2nd paragraph above: #952

victorlin avatar May 25 '22 22:05 victorlin

My thinking was that the sequences and metadata and their downstream processed files in tests/functional/filter/ should be logically consistent such that you could always regenerate the downstream files from these starting points.

+1 for maintaining this property, which is adjacent to snapshot/"known good" testing.

tsibley avatar May 26 '22 23:05 tsibley