Opened 6 years ago

Last modified 5 years ago

#1546 new bug

Coverage reports show more testing needed

Reported by: gropp Owned by:
Priority: major Milestone: future
Component: mpich Keywords:
Cc:

Description (last modified by balaji)

I've updated the script that generates the coverage report to report the fraction of executable lines that are covered. The good news is that we have heavily commented code. The bad news is that only about 85% of the code that we expect to be executed on a regular basis is tested. See http://www.mcs.anl.gov/research/projects/mpich2/todo/coverage/ch3:nemesis/ for the details.

This ticket is a reminder to:

Check the coverage reports for your code

Make sure you make proper use of the source code annotations for code that should be ignored by the coverage tools (e.g., error handling code or experimental code)

Where necessary, expand on the tests to ensure coverage. This is particularly important when a poly-algorithm is used, as this is a major contributor to the number of uncovered lines.

The tool is not perfect at avoiding false positives. Suggestions are welcome for improving it (e.g., one source of uncovered lines is the PMI code; this might not be as important to watch as some of the untested collective algorithms).

Change History (20)

comment:1 Changed 6 years ago by balaji

  • Milestone set to mpich2-1.5
  • Owner set to gropp
  • Status changed from new to assigned

The above link doesn't show anything. Is this already fixed or is the link incorrect?

comment:2 Changed 6 years ago by goodell

  • Owner changed from gropp to goodell
  • Status changed from assigned to accepted

I broke the coverage reports with the recent build system overhaul, so I'm assigning this to myself for now until I can get it fixed.

comment:3 Changed 6 years ago by gropp

For systems without sufficient weak symbol support (e.g., Macs), I've added support that merges the data from the two object file outputs. This update is still in my version of simplemake.in, but will be necessary for the updated coverage support.

comment:4 Changed 6 years ago by goodell

  • Owner changed from goodell to gropp
  • Status changed from accepted to assigned

I think that [0343701070963fc2056879cdaf00337e643ebd63] should basically restore the old coverage functionality that we had before switching to automake. That commit has limitations, see the commit message and new comments.

Merging coverage reports for weak symbol duplicates will be challenging because of gcov limitations. Bill, I'm curious how you solved this problem in your simplemake version...

comment:5 Changed 6 years ago by gropp

I've committed a version of the code to merge the gcov output; this produced reasonable results on my mac. It does leave some debris behind in the form of additional directories and files, but I'd like to leave those around until there's more experience with the merged coverage output.

comment:6 Changed 6 years ago by gropp

The coverage output is now live, thanks to Dave and Rajeev. The results show that there are significant gaps in our test coverage, with more than 20% of the non-error handling code uncovered by any test. Some of this is still mis-marked code (e.g., error handling that isn't correctly marked), but a lot of it is untested code.

There are also directories that aren't included in the output; I'll be updating the coverage script to include more of the relevant directories.

comment:7 Changed 6 years ago by goodell

Something that would help (and that I don't have time to do right now) would be to modify the runtests script to be able to re-run some/all of the tests using the test/mpi/util/nbc_pmpi_adapter.c code. The nonblocking* tests are reasonably good tests of a few different dimensions of the NBC code, but IIRC they don't vary the message size right now, causing lots of coverage gaps in the automated tests. The PMPI adapter was one of the ways that I did manual testing of the NBC code when I first wrote it. I just never saw an easy way to integrate it into the automated build.

comment:8 Changed 6 years ago by gropp

This is actually very easy. It just requires adding a few lines to Makefile.am for each program, as well as to the testlist. I've committed changes that add a few of these; I added a key for "strict MPI required", which is used to skip these tests when running with --enable-strict .

comment:9 Changed 6 years ago by goodell

I was looking for a solution to run the whole test suite (or nearly so) where it wouldn't be as easy to forget to add/remove the nb* version of a test when regular collective tests are added/removed. The change you made in [86cf1f355d34c4d732754a55f16c2471f8d45752] does work fine, I was just hoping to find something less manual and more comprehensive. I was thinking of telling runtests to take a second pass through doing make testing NBC_LIBS='$(top_builddir)/util/nbc_pmpi_adapter.o', with corresponding modifications to Makefile.mtest and util/Makefile.am.

comment:10 Changed 5 years ago by balaji

  • Milestone changed from mpich2-1.5 to future

comment:11 Changed 5 years ago by gropp

The coverage tests are no longer available. What happened to them?

comment:12 Changed 5 years ago by thakur

They are available from http://www.mpich.org/static/cron/

comment:13 Changed 5 years ago by gropp

I get to them from http://wiki.mpich.org/mpich/index.php/Developer_Documentation , which sends me to a different page. That page doesn't point at the correct place.

comment:14 Changed 5 years ago by buntinas

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed.

comment:15 Changed 5 years ago by buntinas

  • Resolution fixed deleted
  • Status changed from closed to reopened

Oops! I fixed the wiki site, not the original ticket.
-d

comment:16 Changed 5 years ago by gropp

Thanks. To address this ticket, the owners of the various routines need to add to the test suite, following the rules for adding new tests. The best way to do that is for everyone to look over the coverage page and identify what tests they need to create. This ticket should be owned by the MPICH lead, not by me, and they should be responsible to ensuring that everyone else knows what needs to be done.

comment:17 Changed 5 years ago by balaji

  • Owner changed from gropp to balaji
  • Status changed from reopened to accepted

It might be impossible to have complete coverage, so I'm not sure we'll ever resolve this ticket the way it stands. It might be more useful to aim for say 99% coverage, and let individual tickets handle missing coverage going forward.

comment:18 Changed 5 years ago by gropp

Note that our current coverage is pretty poor - almost 1/5 of the code is not executed by any test, and this excludes all error handing and debugging code. Much of the unexecuted code is in algorithm branches. One way to generate individual tickets would be to take any instance of the coverage report and generate tickets by area. Note that the coverage reports aren't perfect; the Fortran coverage is sensitive to mismatches in the C and Fortran compilers (read, poor release engineering in the compiler suite).

comment:19 Changed 5 years ago by balaji

  • Description modified (diff)
  • Status changed from accepted to new

comment:20 Changed 5 years ago by balaji

  • Owner balaji deleted
Note: See TracTickets for help on using tickets.