Can't use fopen() from gtest

RESOLVED FIXED in mozilla35

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: ajones, Assigned: ahal)

Tracking

(Depends on 1 bug)

Trunk
mozilla35
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

It would be helpful for writing video related gtests to be able to fopen existing files. I don't mind having to list the files that I'm referencing in the moz.build.
We need to define how we want this to work before we can implement something, keeping in mind that bug 992983 is something we'd like to do eventually (so we shouldn't make it harder for ourselves).

One way might be to finish bug 976733, and then make it so that TEST_HARNESS_FILES.gtest += ['foo'] allows for having a file 'foo' from the current srcdir to be available as fopen("foo") in gtests. (We'd presumably copy those files to $objdir/_tests/gtest and make that the cwd for running gtests.)
I took an action item to look into what would be involved here.
Flags: needinfo?(ahalberstadt)
(In reply to Andrew Halberstadt [:ahal] from comment #2)
> I took an action item to look into what would be involved here.

It would be great if we could either a) reuse whatever we do here for cpp tests that still get run as part of |make check| because they require files (or command-line arguments); or (more involved) b) port those things to gtest and run them there instead.
Jgriffin and I talked to Anthony and he identified this bug as his team's top automation blocker, so I'd rather not block on bug 992983. Luckily we don't need to with ted's proposal. This patch seems to work for me locally, just waiting for a try run:
https://tbpl.mozilla.org/?tree=Try&rev=095e2c89f7f1
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Flags: needinfo?(ahalberstadt)
(In reply to Nathan Froyd (:froydnj) from comment #3)
> It would be great if we could either a) reuse whatever we do here for cpp
> tests that still get run as part of |make check| because they require files
> (or command-line arguments); or (more involved) b) port those things to
> gtest and run them there instead.

I think the approach in the above patch would work equally well for cppunittests. It would just be a matter of getting the harness to launch firefox with a cwd of "$(DEPTH)/_tests/cppunittest". Let's file a separate bug for that though.
(In reply to Andrew Halberstadt [:ahal] from comment #4)
> Jgriffin and I talked to Anthony and he identified this bug as his team's
> top automation blocker, so I'd rather not block on bug 992983. Luckily we
> don't need to with ted's proposal. This patch seems to work for me locally,
> just waiting for a try run:

This seems fine, and it shouldn't cause any problems for implementing bug 992983. We'd simply package _tests/gtest in the test package. You might want to sanity-check that TEST_HARNESS_FILES.gtest works in moz.build and does the right thing in a gtest.
Yep, the try run above adds a file to TEST_HARNESS_FILES and modifies one of the tests to try and open it. I realized this will throw if _tests/gtest doesn't exist though, I'll update the patch to fix that shortly.
Try run looks good. Here's an updated patch that creates _tests/gtest if the directory doesn't already exists (e.g nothing is using TEST_HARNESS_FILES.gtest yet).
Attachment #8493245 - Attachment is obsolete: true
Attachment #8493736 - Flags: review?(ted)
Comment on attachment 8493736 [details] [diff] [review]
Add ability to open support files in gtests

Review of attachment 8493736 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks fine, just one implementation question.

::: testing/gtest/rungtests.py
@@ +116,5 @@
>          OptionParser.__init__(self)
> +        self.add_option("--cwd",
> +                        dest="cwd",
> +                        default=os.getcwd(),
> +                        help="absolute path to directory from which to run the binary")

I wonder if instead of forcing callers to pass this in if we could use the "try to import MozbuildObject and use that if available" technique we're using elsewhere:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/mochitest_options.py#16
Attachment #8493736 - Flags: review?(ted) → review+
Good point. This removes --cwd in favour of just finding it automatically with mozbuild. If mozbuild is not available it defaults back to os.getcwd(). Carrying r+ forward as per irc discussion.
Attachment #8493736 - Attachment is obsolete: true
Attachment #8493936 - Flags: review+
Hm, this new patch fails on osx for some reason:
https://tbpl.mozilla.org/php/getParsedLog.php?id=48708297&tree=Try&full=1#error0

Will investigate tomorrow.
Depends on: 1072452
I'm not really sure why that is failing. It looks like maybe the objdir is structured differently on OSX that breaks an assumption in MozbuildObject.from_environment()? I filed bug 1072452. If it turns out that bug takes a long time to fix, I could always land the previous command line based patch in the interim.
Oh, ugh, this is Universal builds mucking things up.
I doesn't look like bug 1072452 will be resolved anytime soon, and I don't have a mac to look into it myself. So let's just land the original command line version of the patch (which was previously r+'ed, carrying forward).

This version is unbitrotted. Try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1bfa27099678
Attachment #8498370 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a36f6475b2c3

To use this, add the following to a moz.build:
TEST_HARNESS_FILES.gtest += ['foo']                     # fopen('foo')
TEST_HARNESS_FILES.gtest.media.support_files += ['foo'] # fopen('media/support_files/foo')

Let me know if it isn't working for some reason.
https://hg.mozilla.org/mozilla-central/rev/a36f6475b2c3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flags: qe-verify-
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.