Closed
Bug 860827
Opened 12 years ago
Closed 11 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)
(deleted),
patch
|
vd
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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•12 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•12 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•12 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•12 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•12 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•12 years ago
|
||
Attachment #736666 -
Attachment is obsolete: true
Attachment #738985 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 7•12 years ago
|
||
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 8•12 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•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 9•11 years ago
|
||
Test results in https://tbpl.mozilla.org/?tree=Try&rev=c622ed50b499
Comment 10•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 11•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment 12•11 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•11 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•11 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•11 years ago
|
Attachment #747338 -
Flags: review?(bgirard)
Assignee | ||
Comment 15•11 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•11 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•11 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•11 years ago
|
Attachment #747351 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #747338 -
Attachment is obsolete: false
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 18•11 years ago
|
||
Keywords: checkin-needed
Comment 19•11 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•