Last Comment Bug 860827 - Add unit tests for ReadSysFile()
: Add unit tests for ReadSysFile()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla23
Assigned To: Vasil Dimov [:vd]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-11 10:50 PDT by Vasil Dimov [:vd]
Modified: 2013-05-10 01:23 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Unit tests for the newly added functions, using gtest (10.37 KB, patch)
2013-04-11 10:50 PDT, Vasil Dimov [:vd]
no flags Details | Diff | Splinter Review
Unit tests for the newly added functions, using gtest (10.33 KB, patch)
2013-04-11 23:12 PDT, Vasil Dimov [:vd]
dhylands: review+
bgirard: review+
Details | Diff | Splinter Review
Unit tests for the newly added functions, using gtest (12.04 KB, patch)
2013-04-18 05:08 PDT, Vasil Dimov [:vd]
vd: review+
Details | Diff | Splinter Review
Unit tests for the newly added functions, using gtest (12.10 KB, patch)
2013-04-18 08:40 PDT, Vasil Dimov [:vd]
vd: review+
Details | Diff | Splinter Review
Move TEMP_FAILURE_RETRY to FileUtils.h (and rename it) (3.57 KB, patch)
2013-05-09 02:43 PDT, Vasil Dimov [:vd]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review
Move TEMP_FAILURE_RETRY to FileUtils.h (and rename it) (3.71 KB, patch)
2013-05-09 03:17 PDT, Vasil Dimov [:vd]
no flags Details | Diff | Splinter Review

Description Vasil Dimov [:vd] 2013-04-11 10:50:31 PDT
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.
Comment 1 Vasil Dimov [:vd] 2013-04-11 23:12:33 PDT
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 2 Dave Hylands [:dhylands] 2013-04-12 15:46:46 PDT
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 3 Benoit Girard (:BenWa) 2013-04-15 13:57:47 PDT
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 4 Benoit Girard (:BenWa) 2013-04-15 13:59:50 PDT
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)) ?
Comment 5 Vasil Dimov [:vd] 2013-04-17 12:24:26 PDT
(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.
Comment 6 Vasil Dimov [:vd] 2013-04-18 05:08:35 PDT
Created attachment 738985 [details] [diff] [review]
Unit tests for the newly added functions, using gtest
Comment 8 Vasil Dimov [:vd] 2013-04-18 08:40:02 PDT
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).
Comment 9 Vasil Dimov [:vd] 2013-04-23 09:53:31 PDT
Test results in https://tbpl.mozilla.org/?tree=Try&rev=c622ed50b499
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-04-23 10:58:33 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/de74564a8bf3
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-04-23 18:23:22 PDT
https://hg.mozilla.org/mozilla-central/rev/de74564a8bf3
Comment 12 Ethan Hugg [:ehugg] 2013-05-08 20:03:13 PDT
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.
Comment 13 Vasil Dimov [:vd] 2013-05-09 02:40:53 PDT
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.
Comment 14 Vasil Dimov [:vd] 2013-05-09 02:43:57 PDT
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
Comment 15 Vasil Dimov [:vd] 2013-05-09 03:17:32 PDT
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 16 Trevor Saunders (:tbsaunde) 2013-05-09 03:43:57 PDT
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 17 Trevor Saunders (:tbsaunde) 2013-05-09 03:44:55 PDT
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
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-05-09 10:49:19 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3cb79e27d9
Comment 19 Ed Morley [:emorley] 2013-05-10 01:23:58 PDT
https://hg.mozilla.org/mozilla-central/rev/fb3cb79e27d9

Note You need to log in before you can comment on or make changes to this bug.