Add unit tests for ReadSysFile()

RESOLVED FIXED in mozilla23

Status

()

Core
XPCOM
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: vd, Assigned: vd)

Tracking

Trunk
mozilla23
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

4 years ago
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.
Attachment #736369 - Flags: review?(dhylands)
Attachment #736369 - Flags: review?(bgirard)
(Assignee)

Comment 1

4 years ago
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)
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 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 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 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

4 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

4 years ago
Created attachment 738985 [details] [diff] [review]
Unit tests for the newly added functions, using gtest
Attachment #736666 - Attachment is obsolete: true
Attachment #738985 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
(Assignee)

Comment 8

4 years ago
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).
Attachment #738985 - Attachment is obsolete: true
Attachment #739093 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 9

4 years ago
Test results in https://tbpl.mozilla.org/?tree=Try&rev=c622ed50b499
https://hg.mozilla.org/integration/mozilla-inbound/rev/de74564a8bf3
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/de74564a8bf3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Comment 12

4 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

4 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

4 years ago
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
(Assignee)

Updated

4 years ago
Attachment #747338 - Flags: review?(bgirard)
(Assignee)

Comment 15

4 years ago
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.
Attachment #747338 - Attachment is obsolete: true
Attachment #747338 - Flags: review?(bgirard)
Attachment #747351 - Flags: review?(trev.saunders)
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 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

4 years ago
Attachment #747351 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #747338 - Attachment is obsolete: false
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3cb79e27d9
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fb3cb79e27d9
You need to log in before you can comment on or make changes to this bug.