RackSec / desdemona

Data-backed security operations
Eclipse Public License 1.0
2 stars 7 forks source link

Clean up template-generated code, improve test coverage to 90%+ #4

Open lvh opened 8 years ago

lvh commented 8 years ago

Some of the template-generated code is quite duplicated. Some if it is not covered by tests and is only intended for REPL-use, resulting in lower coverage (at time of writing, 75%).

lvh commented 8 years ago

This should be done after @sirsean's work on #21.

ehashman commented 8 years ago

I'd like to tackle this but I'm not actually sure what code is superfluous and needs to be removed vs. code that just needs some tests.

For example, launcher coverage is pretty low. This doesn't appear to be useless template code as @sirsean did a lot of work on it in #21, yet I don't see any mention of tests in that PR. Should these files be exempted from coverage?

sirsean commented 8 years ago

Tests were pretty extensively mentioned in that PR, concluding with lvh: I am merging this despite the codecov failure; the code we care about is tested, the untested code comes straight from the template; we'll be addressing individual coverage improvements after this PR.

Notably almost all the untested code is in various main functions or config maps. Obviously "tested code is better than untested code" but it'd require refactoring those main functions and writing what seem to me like low-value tests.

ehashman commented 8 years ago

(clarification: when I say I didn't see any mention of tests, I meant pertaining to those specific files, not in general)

lvh commented 8 years ago

Testing these all the way is less than stellar, but I do think some of that behavior ought to be tested, since in many cases otherwise you'd get an utterly arcane error (or possibly no error at all). For example, int parsing is something we can check, configuration is something we can check, et cetera. Refactoring would be one way to get easier tests. Another would be mocks that assert what they're called with. Given that we're testing a bunch of gross side effects, they're mostly in the same ballpark.

ehashman commented 8 years ago

With #54 now merged, I think most of the low-hanging fruit test coverage is gone. The next ~25% of test coverage that we'll need to hit our 90% target involved integration testing the Aeron driver, launch-prod-peers, and the -main bit of the sample job. (There's also some less than stellar coverage in logging and metrics but I think closing up #6 will fix that.)

Each of these is going to be tricky, especially since the two launchers block forever when you run them. Ideally I'd like to be able to just run lein test for all of these (and then codecov will actually catch it). But I have not been having much luck finding info on non-web integration testing with Clojure.