search

opensafely-actions / safetab

Safetab outputs two-way tables of descriptive statistics
Other
0 stars 0 forks source link

Rewrite `TestOutputTables` #31

Closed iaindillingham closed 3 years ago

iaindillingham commented 3 years ago

TL;DR There was a bug in the test, not in the function under test. The test has been rewritten, breaking it up into three tests and removing patched function calls.

Previously, this test class had a single test method that heavily relied on patching calls made within the output_tables function (the function under test). An incorrect return value from a patched call suggested that there was a bug; namely, that tables of type groupby-2-way were not being written to the correct directory. By splitting this test class into three test methods, one for each type of table, and by writing an input CSV to a temporary directory rather than by patching, we can confirm that there isn't a bug. As a bonus, we get a better test class, too.

Closes #20

iaindillingham commented 3 years ago

I'm less keen on the rest of the codebase: there are some very long functions, even longer when we consider that they are essentially chained calls 🙂

I'd like to make more assertions about the contents of the files, but in this case, that would probably mean grepping the files*. I think that I'd prefer to follow a test-refactor-test cycle. This PR is the first test; next I was planning to move the loops in output_tables into separate functions; then I was planning to test these functions.

*Not quite true! We could count the number of lines in the log file and also check that when a table has been redacted, its corresponding directory has been created. The former seems like a nice-to-have; the latter seems like an implementation accident.