Created attachment 736369 [details] [diff] [review] Unit tests for the newly added functions, using gtest Opening a new bug as a followup to Bug 819016 (as suggested by dhylands). Add unit tests for ReadSysFile() which was opened in the above mentioned bug.
Created attachment 736666 [details] [diff] [review] Unit tests for the newly added functions, using gtest Address Benoit's suggestions posted to Bug 819016 (https://bugzilla.mozilla.org/show_bug.cgi?id=819016#c32)
Comment on attachment 736666 [details] [diff] [review] Unit tests for the newly added functions, using gtest Review of attachment 736666 [details] [diff] [review]: ----------------------------------------------------------------- Very nice!!!! Thanks for figuring out how to make this work (I wasn't aware of how to do this previously anyways). I was able to do: GTEST_FILTER='ReadSysFile.*' with my B2G-desktop build. I read through TestFileUtils.cpp and all of the tests seem reasonable to me. I'll be able to use this for other tests now.
Comment on attachment 736666 [details] [diff] [review] Unit tests for the newly added functions, using gtest Review of attachment 736666 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. Two small nits, I'm fine landing as-is, just FYI for next time. Thanks for the patch, good work :) ::: xpcom/glue/FileUtils.cpp @@ +118,5 @@ > } > > +/* Define ReadSysFile() only on GONK to avoid unnecessary lubxul bloat. > +Also define it in debug builds, so that unit tests for it can be written > +and run with "./firefox -unittest". */ NIT: This shouldn't include how to run the test suite since that can easily change and be confusing. ::: xpcom/glue/tests/gtest/Makefile.in @@ +16,5 @@ > +LIBXUL_LIBRARY = 1 > +IS_COMPONENT = 1 > + > +LOCAL_INCLUDES = \ > + -I$(srcdir)/../.. \ NIT: 2 space instead of tab for these.
Comment on attachment 736666 [details] [diff] [review] Unit tests for the newly added functions, using gtest Review of attachment 736666 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. Two small nits, I'm fine landing as-is, just FYI for next time. Thanks for the patch, good work :) ::: xpcom/glue/FileUtils.cpp @@ +118,5 @@ > } > > +/* Define ReadSysFile() only on GONK to avoid unnecessary lubxul bloat. > +Also define it in debug builds, so that unit tests for it can be written > +and run with "./firefox -unittest". */ NIT: This shouldn't include how to run the test suite since that can easily change and be confusing. ::: xpcom/glue/tests/gtest/Makefile.in @@ +16,5 @@ > +LIBXUL_LIBRARY = 1 > +IS_COMPONENT = 1 > + > +LOCAL_INCLUDES = \ > + -I$(srcdir)/../.. \ NIT: 2 space instead of tab for these. ::: xpcom/glue/tests/gtest/TestFileUtils.cpp @@ +2,5 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + */ > + > +/* Test ReadSysFile() */ Will this compile and pass in platform where ReadSysFile is not compiled i.e. where !(defined(MOZ_WIDGET_GONK) || defined(DEBUG)) ?
(In reply to Benoit Girard (:BenWa) from comment #4) [...] > ::: xpcom/glue/FileUtils.cpp > @@ +118,5 @@ > > } > > > > +/* Define ReadSysFile() only on GONK to avoid unnecessary lubxul bloat. > > +Also define it in debug builds, so that unit tests for it can be written > > +and run with "./firefox -unittest". */ > > NIT: This shouldn't include how to run the test suite since that can easily > change and be confusing. Removed. > ::: xpcom/glue/tests/gtest/Makefile.in > @@ +16,5 @@ > > +LIBXUL_LIBRARY = 1 > > +IS_COMPONENT = 1 > > + > > +LOCAL_INCLUDES = \ > > + -I$(srcdir)/../.. \ > > NIT: 2 space instead of tab for these. Changed. > ::: xpcom/glue/tests/gtest/TestFileUtils.cpp > @@ +2,5 @@ > > +/* Any copyright is dedicated to the Public Domain. > > + * http://creativecommons.org/publicdomain/zero/1.0/ > > + */ > > + > > +/* Test ReadSysFile() */ > > Will this compile and pass in platform where ReadSysFile is not compiled > i.e. where !(defined(MOZ_WIDGET_GONK) || defined(DEBUG)) ? Hmm, right, it will probably not. I am now testing and will submit an adjusted patch shortly.
Created attachment 738985 [details] [diff] [review] Unit tests for the newly added functions, using gtest
Backed out for Windows bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/62d955cfe160 https://tbpl.mozilla.org/php/getParsedLog.php?id=21954881&tree=Mozilla-Inbound
Created attachment 739093 [details] [diff] [review] Unit tests for the newly added functions, using gtest Change to the previous patch - only define ReadSysFile() and its tests if XP_UNIX is defined (in addition to the other conditions). Before this patch it was defined only for MOZ_WIDGET_GONK. This patch also defines it it for DEBUG which makes the Windows builds to pick it (it does not compile on Win).
Test results in https://tbpl.mozilla.org/?tree=Try&rev=c622ed50b499
https://hg.mozilla.org/integration/mozilla-inbound/rev/de74564a8bf3
https://hg.mozilla.org/mozilla-central/rev/de74564a8bf3
Comment on attachment 739093 [details] [diff] [review] Unit tests for the newly added functions, using gtest Review of attachment 739093 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/tests/gtest/TestFileUtils.cpp @@ +43,5 @@ > + } > + > + offt = 0; > + do { > + ret = TEMP_FAILURE_RETRY(write(fd, (char*)aContents + offt, aContentsLen - offt)); This breaks the --enable-gtest debug build for OSX because TEMP_FAILURE_RETRY is not defined in unistd.h as in Linux. I copied this in from FileUtils.cpp to the top of this file: #undef TEMP_FAILURE_RETRY #define TEMP_FAILURE_RETRY(exp) (__extension__({ \ typeof (exp) _rc; \ do { \ _rc = (exp); \ } while (_rc == -1 && errno == EINTR); \ _rc; \ })) And that seemed to fix the problem.
Hi, to avoid the copy-paste the macro can be moved to the .h file and used from both .cpp files. I will attach a patch here.
Created attachment 747338 [details] [diff] [review] Move TEMP_FAILURE_RETRY to FileUtils.h (and rename it) Followup to Bug 860827 - Add unit tests for ReadSysFile() Move the macro TEMP_FAILURE_RETRY from FileUtils.cpp to FileUtils.h so that it can be used in TestFileUtils.cpp too. Also, rename it to MOZ_TEMP_FAILURE_RETRY to avoid potential clashes with the system defined macro, if it exists. Try results at: https://tbpl.mozilla.org/?tree=Try&rev=59c5327e2d64
Created attachment 747351 [details] [diff] [review] Move TEMP_FAILURE_RETRY to FileUtils.h (and rename it) Followup to Bug 860827 - Add unit tests for ReadSysFile() Move the macro TEMP_FAILURE_RETRY from FileUtils.cpp to FileUtils.h so that it can be used in TestFileUtils.cpp too. Also, rename it to MOZ_TEMP_FAILURE_RETRY to avoid potential clashes with the system defined macro, if it is defined after our macro. Use the system macro by MOZ_TEMP_FAILURE_RETRY if it is present.
Comment on attachment 747338 [details] [diff] [review] Move TEMP_FAILURE_RETRY to FileUtils.h (and rename it) I didn't notice it used to be #undef not ifndef the first time through sorry, but this seems like it can't change behavior and seems reasonable so rs=me
Comment on attachment 747351 [details] [diff] [review] Move TEMP_FAILURE_RETRY to FileUtils.h (and rename it) lets do the first one instead since its actually the one that doesn't change behavior
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3cb79e27d9
https://hg.mozilla.org/mozilla-central/rev/fb3cb79e27d9
or
