GNOME Bugzilla – Bug 781978
Add Google Test C++ test framework
Last modified: 2017-08-07 17:20:06 UTC
Need for unit testing --------------------- I have been thinking that we need to start unit testing GParted. The changes being made to the PipeCapture class in bug 777973 is a good example for the need; refactoring the code while keeping the same interface and fixing tripped over bugs along the way. I see unit testing a good match for testing lower level classes in GParted such as: FS_Info, LUKS_Info, LVM_PV_Info, Mount_Info, PipeCapture, Proc_Partitions_Info, ProgressBar, WSRaid_Info, Utils Having automated unit testing doesn't prevent the need for integration testing, such as that you previous discussed: GParted Forum topic: Idea for Automated Testing of GParted http://gparted-forum.surf4.info/viewtopic.php?id=16432 Unit test framework selection ----------------------------- I had a look at a few C++ unit test frameworks, and quickly put Google test (https://github.com/google/googletest) as the first to try. * Google Test is still being developed. * There is a lot of documentation with it. * There are lots of projects using it, including GNOME projects. ** Therefore there are lots of third part information and answered questions. Therefore I started integrating Google Test and have so far found no reason consider using any other unit testing framework. Google Test integration mechanism --------------------------------- Google Test says that it should not be build as a system library, but rather build with your application being tested: Google Test FAQ: Why is it not recommended to install a pre-compiled copy of Google Test (for example, into /usr/local)? https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#why-is-it-not-recommended-to-install-a-pre-compiled-copy-of-google-test-for-example-into-usrlocal Google Test is under the 3-clause BSD license: * 3-Clause BSD License https://opensource.org/licenses/BSD-3-Clause * Google Test LICENSE file https://github.com/google/googletest/blob/master/googletest/LICENSE I found this example project of how to integrate Google Test into an autotools project and other useful answers: * minimal-gtest-autotools https://github.com/octol/minimal-gtest-autotools * How can I use Google Test with my project that builds via autotools? http://stackoverflow.com/questions/35998856/how-can-i-use-google-test-with-my-project-that-builds-via-autotools Therefore I choose to copy the portions of Google Test that I needed directly into GParted. Google Test could have been integrated using git submodule, but that is just a little bit more complicated with greater dependencies. It seemed easier just to copy the needed files into our package. Discussion ---------- This is so you can see how I came to the decisions I made and see what I am working towards. Will only be added minimal unit testing to demonstrate everything hangs together. More can be added later as needed. Thanks, Mike
Hi Mike, I am in favour of automated testing of software. I haven't investigated unit test software so I'll rely on your recommendations in this area. Some questions I have are: 1) Do we need to include the whole googletest repository in the gparted source code repository? More specifically do we need the whole googletest repository checked into the gparted git repository, or only a few key files? 2) Do you foresee any issues with license compatibility? From a quick look it appears that the BSD 3 clause license is compatible with the GPL [1]. [1] https://en.wikipedia.org/wiki/License_compatibility#GPL_compatibility 3) Will the gparted code still be able to be compiled without googletest? Regards, Curtis
Created attachment 350877 [details] [review] Add Google Test framework for unit testing (draft 1) Hi Curtis, 1) Yes only a subset of googletest repository is being copied into GParted. Specifically these files and directories from the googletest/ sub-directory, from the latest release 1.8. LICENSE README.md include/** src/** https://github.com/google/googletest/tree/ec44c6c1675c25b9827aacd08c02433cccde7780/googletest 2) I don't see any licensing issues. GNU project specifically document the 3-clause BSD license as being compatible with the GPL. https://www.gnu.org/licenses/license-list.html#ModifiedBSD 3) Yes gpartedbin will build without googletest libraries. It still builds exactly the same as it does before using: ./configure make sudo make install Building and running the unit tests is done by the Autoconf testing framework. I.e.: make check Attached is patchset draft 1 so you can where I have got to. The last commit doesn't work at the moment so if you want a successful "make check" use the previous commit. Also haven't yet confirmed that "make distcheck" works. That is all the new files are correctly included in an archive and "make check" works in that too. Thanks, Mike
Hi Mike, Thanks for answering the questions. So far I do not foresee any issues with including googletest for unit testing. Thanks also for providing a patch set to show where you are at. When attempting to apply the patch set to the current master I see the following error messages: $ git am bug781978-googletest.patch Applying: Add Google Test 1.8.0 files (#781978) .git/rebase-apply/patch:9237: new blank line at EOF. + warning: 1 line adds whitespace errors. Applying: Update README with the inclusion of Google Test framework (#781978) Applying: Exclude 2 gtest files from translation (#781978) Applying: Switch to conditional appending of SUBDIRS in top-level Makefile.am error: patch failed: Makefile.am:1 error: Makefile.am: patch does not apply Patch failed at 0004 Switch to conditional appending of SUBDIRS in top-level Makefile.am The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". $ I worked around this first message, used "git am --continue" only to encounter another issue in the next patch. $ emacs Makefile.am $ git add Makefile.am $ git am --continue Applying: Switch to conditional appending of SUBDIRS in top-level Makefile.am Applying: Add building of Google Test libraries (#781978) error: patch failed: Makefile.am:1 error: Makefile.am: patch does not apply Patch failed at 0005 Add building of Google Test libraries (#781978) The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". $ Are you able to apple the draft patch set against the current gparted master? Thanks, Curtis
Created attachment 350905 [details] [review] Add Google Test framework for unit testing (draft 2) Hi Curtis, Sorry for patchset draft 1 not applying. I had built the patchset on top of a tidyup patch which added a line to the top of ./Makefile.am and therefore provided context which didn't match for 3 of the 8 patches. Here is the same patchset rebased on top of master so it will apply. * Just ignore a whitespace error for now. I'll look at that later. * Also as before the last commit doesn't pass 'make check' so drop that to test for now. Mike
Hi Mike, Thank you for posting the updated patch set draft 2. I took a look at the patch set draft 2 sections 2/8 though 7/8 and these look good to me. I see that these implement a dummy test that should succeed. 'make check' worked for me. As you indicated earlier, 'make distcheck' might not work, and it didn't work for me. The error message I see (which you can likely reproduce) is: <snip> make[3]: Entering directory '/home/gedakc/workspace/gparted/gparted-0.28.1-git/_build/sub/tests' g++ -DHAVE_CONFIG_H -I. -I../../../tests -I.. -I../../../include -I../../../lib/gtest/include -DGTEST_HAS_PTHREAD=1 -Wall -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -g -O2 -std=gnu++11 -MT test_dummy.o -MD -MP -MF .deps/test_dummy.Tpo -c -o test_dummy.o ../../../tests/test_dummy.cc mv -f .deps/test_dummy.Tpo .deps/test_dummy.Po make[3]: *** No rule to make target '../../../lib/gtest/lib/libgtest_main.la', needed by 'test_dummy'. Stop. make[3]: Leaving directory '/home/gedakc/workspace/gparted/gparted-0.28.1-git/_build/sub/tests' Makefile:894: recipe for target 'check-am' failed <snip> I also tried 'make dist' and noticed that the lib/gtest stuff is included in the tarball. 'make check' does work with the released tarball. Is this something we wish to include in release tarballs? Or is the lib/gtest stuff something that is only needed in development and for testing? I haven't looked at 1/8 which includes all the googletest required code yet. Other than that, thinks are looking good from my perspective. Curtis
Created attachment 351122 [details] [review] Add Google Test framework for unit testing (draft 3) Hi curtis, Yes I think that including the unit testing in the release tarballs is something that we should do. As a couple of examples both parted and gstreamermm include their unit test scripts in their tarball releases. I suspect every project that has unit testing includes them in their releases. RPMs even support running tests during package building and is used by the parted RPM. https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25check_section http://pkgs.fedoraproject.org/cgit/rpms/parted.git/tree/parted.spec?h=f26#n157 Also here is patchset draft 3. It contains working unit tests for the BlockSpecial class. However the tests are incomplete and does contain *deliberate* test failures to see what it looks like. Thanks, Mike
Hi Mike, Thanks for sharing your thoughts and the links to fedora's preferences for tests and checks. I am onside with including the checks with the release tarball. Thanks, Curtis
Created attachment 351645 [details] [review] Add Google Test framework for unit testing (v1) Hi Curtis, Here is patchset v1. Test and review when ready but don't push to master just yet. I want to add unit testing of PipeCapture into bug 777973 first to check this base is about right. (I'm sure it will be though). Note that the white space error applying patch number 1 is caused by the blank line at the end this file from the Google Test project: include/gtest/internal/gtest-internal.h We should include pristine files from Google Test and not correct this. Also fixes the make distcheck you observed in comment 5. Compared to your initial review from comment 5 existing patches have only been cosmetically changed, however new patches have been added to the set. Diff the patchsets for full details. I've successfully run: ./autogen.sh make make check make distcheck on CentOS 6 and Fedora 24. Thanks, Mike
Hi Mike, Patch set v1 from comment #8 looks good to me. I assumed that the google test code is okay, and only checked for things like copyright notices. Using the patch set in a distribution tarball, I have successfully run: ./configure --disable-scrollkeeper make make check make distcheck on the following distros: debian 8 kubuntu 16.04 opensuse 42.1 As requested I will hold off on committing the patch set until I get either your go ahead, or you publish an updated patch set. Thanks, Curtis
Created attachment 352672 [details] [review] Add Google Test framework for unit testing (v2) Hi Curtis, Here's patchset v2. Compared to v1 a couple of patches have been updated adding needed entries into .gitingore and 3 tidyup patches added. I have built working PipeCapture unit test for bug 777973 on top of this patchset. Commit to master when reviewed. Commit order: this first, and patchset from bug 777973 second. Thanks, Mike
Hi Mike, Using patch set v2 from comment #10, I was running a 'make check' and came across the following warning: <snip> g++ -DHAVE_CONFIG_H -I. -I.. -I../include -I../lib/gtest/include -DGTEST_HAS_PTHREAD=1 -Wall -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -std=c++11 -pthread -I/usr/include/gtkmm-2.4 -I/usr/lib/x86_64-linux-gnu/gtkmm-2.4/include -I/usr/include/atkmm-1.6 -I/usr/include/gtk-unix-print-2.0 -I/usr/include/gtk-2.0 -I/usr/include/gdkmm-2.4 -I/usr/lib/x86_64-linux-gnu/gdkmm-2.4/include -I/usr/include/giomm-2.4 -I/usr/lib/x86_64-linux-gnu/giomm-2.4/include -I/usr/include/pangomm-1.4 -I/usr/lib/x86_64-linux-gnu/pangomm-1.4/include -I/usr/include/glibmm-2.4 -I/usr/lib/x86_64-linux-gnu/glibmm-2.4/include -I/usr/include/cairomm-1.0 -I/usr/lib/x86_64-linux-gnu/cairomm-1.0/include -I/usr/include/sigc++-2.0 -I/usr/lib/x86_64-linux-gnu/sigc++-2.0/include -I/usr/include/gtk-2.0 -I/usr/lib/x86_64-linux-gnu/gtk-2.0/include -I/usr/include/gio-unix-2.0/ -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libpng12 -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/libpng12 -I/usr/include/pango-1.0 -I/usr/include/harfbuzz -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/freetype2 -g -O2 -std=gnu++11 -MT test_BlockSpecial.o -MD -MP -MF .deps/test_BlockSpecial.Tpo -c -o test_BlockSpecial.o test_BlockSpecial.cc test_BlockSpecial.cc: In function ‘std::__cxx11::string GParted::get_link_name()’: test_BlockSpecial.cc:143:32: warning: suggest parentheses around assignment used as truth value [-Wparentheses] while ( dentry = readdir( dir ) ) ^ mv -f .deps/test_BlockSpecial.Tpo .deps/test_BlockSpecial.Po <snip> Do you see this warning as well on your development system? Thanks, Curtis
Hi Curtis, Yes I did see the warning. It is GCC trying to protect us from doing this: while ( var = expr && bool_expr || bool_expr ) It wants an extra set of parentheses around var = expr because the above means this without them: while ( var = ( expr && boo_expr || bool_expr ) ) However it is not necessary in this case with a single var = expr being used as a boolean conditional expression. I was just going for ignoring the warning. Alternatively the code could be changed to have a double set of parentheses like this: while ( ( dentry = readdir(dir) ) ) Otherwise the suggested compiler flag could be added to silence the warning, but that would prevent getting the warning else where too, where it might be necessary. I will there was a suitable comment to inform the compiler that this case is safe. I suspect that there is no such comment. I have just decided that it would probably be best to do something like this: // Silence GCC [-Wparentheses] warning with double parentheses while ( ( dentry = readdir( dir ) ) ) Mike
Hi Mike, I can make that change and continue with my testing. Thanks, Curtis
Hi Mike, Patch set v2 from comment #10 (with change from comment #12) looks good to me. Using the patch set in a distribution tarball, I have successfully run: ./configure --disable-scrollkeeper make make check make distcheck on the following distros (most different from comment #9): debian 7 fedora 23 kubuntu 16.04 ubuntu 14.04 Based on these successful tests, I have committed the patch set to the git repository for inclusion in the next release of GParted. The relevant git commits can be viewed at the following links: Add Google Test 1.8.0 files (#781978) https://git.gnome.org/browse/gparted/commit/?id=87f7170a55e1a04aeca5412ebb97e0fe0bfe7b4c Update README with the inclusion of Google Test framework (#781978) https://git.gnome.org/browse/gparted/commit/?id=cbab9849b1140b7c55a06ef0db86f3806fcf55a7 Exclude 2 gtest files from translation (#781978) https://git.gnome.org/browse/gparted/commit/?id=0841b0274b57852fce531c17884c120dac65098f Switch to conditional appending of SUBDIRS in top-level Makefile.am (#781978) https://git.gnome.org/browse/gparted/commit/?id=4d6823a20a9c7481a815016024dc427cc33aa7c5 Add building of Google Test libraries (#781978) https://git.gnome.org/browse/gparted/commit/?id=81b104928b35d46faf200dc3265ac2254b6246c5 Add unit test suites with initial successful dummy test (#781978) https://git.gnome.org/browse/gparted/commit/?id=dceb293f15e60ee6a932499b6c63b2a6fb84f38a Fix make distcheck failure from missing libgtest_main.la (#781978) https://git.gnome.org/browse/gparted/commit/?id=dbd5cd2ca80dcc09de9c40878826e8babc4cbc07 Add unit tests for BlockSpecial constructors and internal caching (#781978) https://git.gnome.org/browse/gparted/commit/?id=c37be28148ec6c6b2b9be4cf097f4d20513c9a14 Add BlockSpecial operator==() tests (#781978) https://git.gnome.org/browse/gparted/commit/?id=debbd811b8a642de786e134bfc6b5b84b373adaa Add test for bug 771670 in BlockSpecial::operator==() (#781978) https://git.gnome.org/browse/gparted/commit/?id=f4008092dd99969299bef05b975a00f654026c4c Add BlockSpecial operator<() tests (#781978) https://git.gnome.org/browse/gparted/commit/?id=003d1b54c7b25c7e0aa367cb46c8ba60c5abde7a Improve diagnostics of failed BlockSpecial operator tests (#781978) https://git.gnome.org/browse/gparted/commit/?id=72e81622f3fc694ba9a86381de783d7a43bbbf76 Implement libtoolize suggestion setting ACLOCAL_AMFLAGS https://git.gnome.org/browse/gparted/commit/?id=9af41093f8856deea75c572e3195744334e85abb Remove old .cvsignore files https://git.gnome.org/browse/gparted/commit/?id=69d1bbcf8ff5a8102420aa88579d0b77f2fce22e Remove unused build definition GPARTED_DATADIR https://git.gnome.org/browse/gparted/commit/?id=7c51b62958a2ffa0ae9276910f7f5238d772f716 Curtis
Created attachment 354194 [details] [review] Silence subdir-objects warning from automake 1.14 Hi Curtis, Here's the fix for a cosmetic subdir-objects warning from automake 1.14 and later which is introduced with this patchset. Thanks, Mike
Thank you Mike for this patch to address a warning with newer automake versions. The patch in comment #15 looks good to me and has been committed to the git repository for inclusion in the next release of gparted. The relevant git commit can be viewed at the following link: Silence subdir-objects warning from automake 1.14 (#781978) https://git.gnome.org/browse/gparted/commit/?id=f31808989ab7562ad411911c8b2dc16958708be8
This enhancement was included in the GParted 0.29.0 release on August 7, 2017.