860827 - Add unit tests for ReadSysFile()

Status: RESOLVED FIXED

Description

[Vasil Dimov [:vd]]

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]]

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]]

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)]

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)]

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]]

(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]]

Created attachment 738985 [details] [diff] [review]
Unit tests for the newly added functions, using gtest

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Vasil Dimov [:vd]]

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]]

Test results in https://tbpl.mozilla.org/?tree=Try&rev=c622ed50b499

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de74564a8bf3

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de74564a8bf3

Comment 12

[Ethan Hugg [:ehugg]]

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]]

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]]

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]]

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)]

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)]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3cb79e27d9

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fb3cb79e27d9