Menu
Logged-In As
ACCOUNTNot Logged In
Create an utility library (LIBBU) API unit test for bomb.cBRL-CAD
Status: ClosedTime to complete:
100 hrs
Mentors: Mandeep Kaur, Daniel_R
There are more than 300 library functions in our core LIBBU library. As a core library used by nearly every one of BRL-CAD's tools, testing those functions for correct behavior is important.
References:
- include/bu.h
- src/libbu/bomb.c
- src/libbu/tests/*.c
Code:
- src/libbu/tests/bomb.c
- src/libbu/tests/CMakeLists.txt
This task involves implementing a new unit test for any of LIBBU's source files that do not already have a unit test defined. The test should run all of the public functions and be hooked into our build system. We have lots of existing unit tests to follow as an example.
Uploaded Work
| File name/URL | File size | Date submitted | |
|---|---|---|---|
| bomb_unit_test_add | 5.4 KB | December 12 2014 21:31 UTC | |
| bomb_test | 3.2 KB | December 13 2014 08:15 UTC | |
| _with_undef_ | 2.9 KB | December 14 2014 07:34 UTC | |
| _without_undef_ | 2.9 KB | December 14 2014 07:34 UTC | |
| bu_bomb_revision | 3.4 KB | December 14 2014 13:27 UTC | |
| bu_bomb_macro | 3.5 KB | December 14 2014 16:54 UTC |
I would like to work on this task.
This task has been assigned to Marc Tannous. You have 100 hours to complete this task, good luck!
Looked into the bombs.c file, the two functions I came up with that required testing were :
- bu_exit, which should exit the program and return the status sent as a parameter to it.
- bu_bomb, which I tested by seeing if it gets to the end and does not create an infinite lop.
The bu_bomb test was easy to implement, however when I tried returning bu_exit's status, I got an error while compiling because _exit literally kills the process.
After consultation with the mentors over IRC ( Erik, maths, mihai, and andrei ) they suggested creating another process, a child process that would exit, and a parent process that would get its return status. Was a really neat trick, spent a few hours looking into functions and the whole idea behind it, seemed really really nice, I loved it.
Attached is the diff file, please ignore modifications outside of bu_bomb and cmakelists.txt, as these were done for other tasks and not commited to the latest version.
Regards,
Marc
The work on this task is ready to be reviewed.
Marc,
This looks pretty good and the approach is sound, but there are a few minor problems with the patch that you should fix before we can call this done.
First is the extraneous content in the patch file. We use tools that automatically make the changes prescribed within a patch file, so if you include extra stuff, we get those extra changes made and then we have to spend time undoing them. It's far better and easier and less time to everyone all around if you fix the problem on your end. You can either 1) revert the changes to those files - svn revert -R src/libbn, or 2) only create a patch file of the files you modified - svn diff src/libbu my.patch, or 3) edit your patch file and remove the offending content - remove everything from an Index line to just before the next Index line.
Second issue is the inconsistent style in the file. You don't need to conform to our rather rigorous style guide for GCI (see HACKING file if you would like to conform, and required to attain commit rights), but the file should at least be self-consistent. For example, in some places you put curly braces on the next line and in other places you put it on the same line; in some places you have spaces after commas and in others you do not; etc. If you want a quick helper, just look at any of our other files.
Third, you're including far too many headers. Only include those that you directly need. You definitely don't use stdarg.h, debug.h, and undoubtedly several others.
Lastly is the issue of portability. This file will result in a compilation failure on Windows. At a minimum, you'll want to wrap the unistd.h header in a #ifdef HAVE_UNISTD_H wrapper along with the entire contents of the test_bu_exit() function.
p.s. your coding skills are getting noticeably better...
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.
The work on this task is ready to be reviewed.
- Consistency in file
All commas are now followed by a space, all the curly braces are on a new line.
- Headers
Reduced the number of included headers, only included unistd.h when not on windows.
- Extraneous content
Removed
- Portability
All is now wrapped in an if, that returns 0 on a windows machine and executes the test with unistd.h declared if on any other machine.
I compiled it, everything is working as intended.
Regards,
Marc
Marc,
Looking better, but the introduction of platform-specific preprocessor symbols will break our build (we don't allow the addition of any without simultaneous removal). You don't need to check for windows (and testing for windows is not correct anyways). Windows was but an example in my first reply. It's can really all be made contingent on HAVE_UNISTD_H being defined.
Consequently, you also pulled the include for unistd.h down into the test_bu_exit() function which is syntactically incorrect (this should have even given you an error.
You want to test that this unit test works both with and without unistd.h .. you can do that by temporarily adding a #undef HAVE_UNISTD_H after all the headers and making sure it gets to the fallback case (then removing the #undef).
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.
Added the ifdef condition, ran "make" in brlcad-build to see if there are any issues.
"BRL-CAD Release 7.25.0, Build 20141213
Elapsed compilation time: 3 minutes 57 seconds
Elapsed time since configuration: 13 hours 54 minutes 29 seconds "
To test the else, I added an undef HAVE_UNISTD_H and then ran "make" again.
"BRL-CAD Release 7.25.0, Build 20141213
Elapsed compilation time: 3 minutes 5 seconds "
Both patches are added, the one with undef is not to be commited as it serves no purpose but testing the other scenario.
Regards,
Marc
The work on this task is ready to be reviewed.
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.
After my last batch of updates, I spoke to Ch3ck via IRC.
He enlightened me about some of the macro processors principles and suggested that I define the function inside an IFDEF for unistd.h . However, I cannot #define a function inside another macro proessor as the results of my entire evening showed ( realized that after 2 hours of trying to fix the various compiler issues ), and defined it as a function.
I have succesfully compiled this on the VM, the tests work fine and I am exhausted.
This might seem as simple code, but I swear to God I took all the possible options of the code until reaching this one that works, is portable, defines as little libraries as possible and GETS THROUGH COMPILATION. I have spent my entire weekend on figuring out how macroprocessors work and I can say that I've learned a lot, but also that I hope you like this final version of my source, and if you have any questions I'm probably on IRC.
Have a good day/evening, sorry for taking so long to solve such a simple task,
The work on this task is ready to be reviewed.
We appreciate the effort you've put into this. I'll handle any further problems with this.
Thanks
Congratulations, this task has been completed successfully.