Create two unit tests for the 'h3' cli app #556
Conversation
CMakeLists.txt
Outdated
| message("${name}_test${test_number} - ${argfile}") | ||
| endif() | ||
|
|
||
| # No coverage possible for H3 CLI testing |
There was a problem hiding this comment.
I assume coverage should be somehow possible (would need to configure the binaries to produce coverage, and include them in the considered source paths)?
There was a problem hiding this comment.
I'll study it, but okay if that's a separate PR?
|
Not a huge fan of this approach - seems like it will bloat the CMakeLists.txt file, and I don't like the idea that we're expanding the purpose of that file beyond build tasks. Other options:
|
|
@nrabinowitz Another option would be to define the tests in a separate CMake file but still in that framework. |
I personally like this compromise. The CMakeLists file does already handle a lot of stuff, and this would help us organize not just the test logic but other things like the fuzzers. |
That seems okay by me. The main issue for me is just that the rest of the CMake file doesn't have actual data or need the kind of review that test cases do, whereas this does. Separating it, ideally with a header comment, should be fine (and I agree that other parts could be separated out as well). |
…fully this fixes that?
| add_h3_test(testH3CellAreaExhaustive src/apps/testapps/testH3CellAreaExhaustive.c) | ||
|
|
||
| add_h3_cli_test(testCliCellToLatLng "cellToLatLng -c 8928342e20fffff" "37.5012466151, -122.5003039349") | ||
| add_h3_cli_test(testCliLatLngToCell "latLngToCell --lat 20 --lng 123 -r 2" "824b9ffffffffff") |
There was a problem hiding this comment.
I still feel like there's a substantive difference between tasks that run a test and tasks that are tests. This is fine for now, but I don't see how it scales up to a large number of tests - I'd much rather have inputs and outputs defined outside of CMake (in a text file, in yaml, or even as separate lines in a script file).
Not adding any extra functionality to the app until we have tests for it. Wanted to put this approach in front of you guys to see what you think.
I can't figure out how to do coverage tests on the CLI app, but I don't think that's too big of a problem?