TDycores-Project / TDycore

BSD 2-Clause "Simplified" License
4 stars 0 forks source link

Improved CMocka demo: pkg-config support, isolated processes #146

Closed jeff-cohere closed 3 years ago

jeff-cohere commented 3 years ago

Here's another illustration of how we might use CMocka to write unit tests. This PR uses pkg-config to determine whether CMocka is available, and then provides targets for running unit tests. These targets aren't finished yet--they just run all the tests in one process. I wanted to focus for now on how one might poll a test executable for the number of tests and then run them one at a time.

To see all this, try the following:

# Build the unit tests. This target also runs the tests in one process, which we don't want, but whatever.
make unit-tests

# Ask our unit test exe how many tests it has (should be 3).
./src/tests/test_tdyinit count

# Try running the tests individually by 0-based index.
./src/tests/test_tdyinit 0
./src/tests/test_tdyinit 1
./src/tests/test_tdyinit 2

# What happens here?
./src/tests/test_tdyinit 3

One drawback of this approach is that command line arguments are not available to unit tests--they are interpreted as a user "talking to" the test harness. Arguably, this isn't a drawback, since parameterizing unit tests this way can be hazardous in some circumstances.

If we like this approach, I can wrap this machinery up in a header file that we can just use for our other tests.

codecov-io commented 3 years ago

Codecov Report

Merging #146 (c77e36e) into master (23af914) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #146   +/-   ##
=======================================
  Coverage   67.73%   67.73%           
=======================================
  Files           7        7           
  Lines        1221     1221           
=======================================
  Hits          827      827           
  Misses        394      394           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 23af914...c77e36e. Read the comment docs.

jeff-cohere commented 3 years ago

@jedbrown , have you had a chance to look this over? If we want to adopt this (or a similar) approach that uses CMocka, I can add it to the Docker image that we're now using so it's around for our autotesting. If we think unit testing with MPI is too complicated, maybe we can articulate a different approach to constructing tests that doesn't always rely on "gold" files.

bishtgautam commented 3 years ago

I'm taking myself off the reviewer list as I'm not qualified to review it.

jeff-cohere commented 3 years ago

This PR is ready for review. I wasn't able to make things "turn-key" for cmocka MPI test programs in the same way as for serial programs, but I've tried to address @jedbrown 's concerns from #139 in a non-intrusive way. it's still true that test output can be confusing in the sense that each process prints it, but addressing that would be a bit more invasive.

codecov-commenter commented 3 years ago

Codecov Report

Merging #146 (65ef4f0) into master (4c6a0d1) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 65ef4f0 differs from pull request most recent head b81660c. Consider uploading reports for the commit b81660c to get more accurate results Impacted file tree graph

@@           Coverage Diff           @@
##           master     #146   +/-   ##
=======================================
  Coverage   67.73%   67.73%           
=======================================
  Files           7        7           
  Lines        1221     1221           
=======================================
  Hits          827      827           
  Misses        394      394           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4c6a0d1...b81660c. Read the comment docs.

jeff-cohere commented 3 years ago

Hi @jedbrown and @bishtgautam ,

We've got another PR (#174) that uses this cmocka machinery to illustrate how we can create labels for boundary conditions in meshes. If no one objects, I'd like to get this merged so we can try writing more unit tests and see if e.g. they help us understand some of the PETSc concepts we've been discussing.

jeff-cohere commented 3 years ago

Thanks, Jed, for approving and for the feedback. Both of those items are doable, and I'll add them in another PR soon.