Closed
Bug 860827
Opened 8 years ago
Closed 8 years ago
Add unit tests for ReadSysFile()
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: vd, Assigned: vd)
Details
Attachments
(2 files, 4 obsolete files)
12.10 KB,
patch
|
vd
:
review+
|
Details | Diff | Splinter Review |
3.57 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #736369 -
Flags: review?(dhylands)
Attachment #736369 -
Flags: review?(bgirard)
Assignee | ||
Comment 1•8 years ago
|
||
Address Benoit's suggestions posted to Bug 819016 (https://bugzilla.mozilla.org/show_bug.cgi?id=819016#c32)
Attachment #736369 -
Attachment is obsolete: true
Attachment #736369 -
Flags: review?(dhylands)
Attachment #736369 -
Flags: review?(bgirard)
Attachment #736666 -
Flags: review?(dhylands)
Attachment #736666 -
Flags: review?(bgirard)
Comment 2•8 years ago
|
||
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.
Attachment #736666 -
Flags: review?(dhylands) → review+
Comment 3•8 years ago
|
||
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.
Attachment #736666 -
Flags: review?(bgirard) → review+
Comment 4•8 years ago
|
||
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)) ?
Assignee | ||
Comment 5•8 years ago
|
||
(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.
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #736666 -
Attachment is obsolete: true
Attachment #738985 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 7•8 years ago
|
||
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
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 8•8 years ago
|
||
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).
Attachment #738985 -
Attachment is obsolete: true
Attachment #739093 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 9•8 years ago
|
||
Test results in https://tbpl.mozilla.org/?tree=Try&rev=c622ed50b499
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/de74564a8bf3
Flags: in-testsuite+
Keywords: checkin-needed
Comment 11•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/de74564a8bf3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment 12•8 years ago
|
||
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.
Assignee | ||
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Attachment #747338 -
Flags: review?(bgirard)
Assignee | ||
Comment 15•8 years ago
|
||
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.
Attachment #747338 -
Attachment is obsolete: true
Attachment #747338 -
Flags: review?(bgirard)
Attachment #747351 -
Flags: review?(trev.saunders)
Comment 16•8 years ago
|
||
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
Attachment #747338 -
Flags: review+
Comment 17•8 years ago
|
||
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
Attachment #747351 -
Flags: review?(trev.saunders)
Assignee | ||
Updated•8 years ago
|
Attachment #747351 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #747338 -
Attachment is obsolete: false
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3cb79e27d9
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•