Fix any failing unit test #9BRL-CAD
Status: ClosedTime to complete: 100 hrs Mentors: Gauravjeet Singh, DishankTags: c, validation, code, testing, verification

We have a lot of unit tests. These tests help ensure proper functionality. Sometimes we add new tests and either 1) the test itself has an error or 2) the test exposes an error in our implementation. We have a few right now that report an error and someone needs to figure out if it's because of reason #1 or reason #2, and then fix the error so the test succeeds.

We will keep adding more tasks like this until there are no more failures. There are also tasks for adding more unit tests.

This task involves compiling BRL-CAD, running "make test", and fixing any of the failing unit tests properly. Your fix should repair all of the issues in any given file.

You'll want to first build BRL-CAD from a source tree checkout (see http://brlcad.org/wiki/Compiling) either directly from our repository or using the provided virutal machine (see http://brlcad.org/wiki/Deuces for setup instructions). Compile BRL-CAD, then run our unit tests ("make test" on unix, or select the "test" target on other platforms).

Make sure any changes you make compile cleanly and fix the error.

Submit a summary analysis / description of what the error you fixed was, why it was failing, and what you did to fix it.

Submit your changes as a patch file (you can run "svn diff my_changes.patch").

References:
  • http://brlcad.org/wiki/Compiling
  • http://brlcad.org/wiki/Deuces
Modify:
  • The tests are all in subdirectories, so you can find them easily:
  • src/libbu/tests
  • src/libbn/tests
  • src/librt/tests
  • etc... fix one that is failing.
Uploaded Work
File name/URLFile sizeDate submitted
task-29-fix-bu-semaphore-tests.c486 bytesDecember 13 2014 15:10 UTC
Comments
Andromeda Galaxyon December 13 2014 15:04 UTCTask Claimed

I would like to work on this task.

Harmanpreet on December 13 2014 15:09 UTCTask Assigned

This task has been assigned to Andromeda Galaxy. You have 100 hours to complete this task, good luck!

Andromeda Galaxyon December 13 2014 15:13 UTCWhere this test was failing.

Previously, the bu_semaphore -P* tests would fail on machines with fewer cores than were requested with -P.  Inspecting the bu_semaphore source showed that in bu_debug BU_DEBUG_PARALLEL, they would attempt to continue anyway.  This test should, therefore, fix at least one bu_semaphore test on machines with fewer than three cores.

Andromeda Galaxyon December 13 2014 15:13 UTCReady for review

The work on this task is ready to be reviewed.

Sean on December 14 2014 04:21 UTCAm I missing something

What is causing the setting of a debug flag to make the test work?  On the surface, there's not nearly enough information to say whether this is a good approach.  More summary info please?


On the surface, that would imply that we have parallel code that might fail simply by not utilizing all available cpus.


 

Sean on December 14 2014 04:21 UTCTask Needs More Work

One of the mentors has sent this task back for more work. Talk to the mentor(s) assigned to this task to satisfy the requirements needed to complete this task, submit your work again and mark the task as complete once you re-submit your work.

Andromeda Galaxyon December 14 2014 05:13 UTCTest fixing approach

parallel_test() calculates the number that the counter should eventually be incremented to by multiplying the number that each thread increments it by and the number of threads started.  However, when not in debug mode, bu_parallel() limits the number of threads that it creates to be at most the number of cpus available to the system.  This means that when the test tries to run with more threads than there are available cpus (without the debug flag, previous to this patch), the actual number that the counter would be incremented by would be avail_cpus * reps instead of ncpus * reps.  Setting the debug flag, as this patch does, tells bu_parallel to use the requested number of threads even when that number is more than the number of available cpus; this is better than having the test simply calculate the number of additions that would be performed by looking at the number of available cpus because it causes the test to be able to test the code paths used for multiple threads.

Andromeda Galaxyon December 14 2014 05:13 UTCReady for review

The work on this task is ready to be reviewed.

Sean on December 15 2014 04:56 UTCTask Closed

Congratulations, this task has been completed successfully.

Sean on December 15 2014 05:04 UTCgood explanation, comment lacking

Your explanation definitely clarified what is going on, but there is definitely information lacking in the patch comment.  Simply saying "makes the test work" isn't saying what the issue is or why this particular bit flag helps and becomes a mystery for future devs.


Something like "setting the parallel debug flag allows for more threads to be created than are actually available on the host system.  this permits multithreaded testing even on single core systems (for example)."


Also, your comment style does not match our multiline comment convention (see HACKING style commentary on block comments).


 

Andromeda Galaxyon December 15 2014 05:16 UTCUpdate the comment

That's true, sorry! I had been intending to put more information in the commit message, but I'll add it to the file as well.  Sorry about the comment convention, I normally try to be careful but I'd just previously been working on the y2038 code that all uses a different convention.