1054809 - Can't use fopen() from gtest

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 1

[(not currently active) Ted Mielczarek]

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

Comment 2

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

I took an action item to look into what would be involved here.

Comment 3

[Nathan Froyd [:froydnj]]

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

Comment 4

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

Attachment: Add ability to open support files in gtests

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

Comment 5

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

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

Comment 6

[(not currently active) Ted Mielczarek]

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

Comment 7

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

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.

Comment 8

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

Attachment: Add ability to open support files in gtests

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

Comment 9

[(not currently active) Ted Mielczarek]

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

Comment 10

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

Attachment: Add ability to open support files in gtests

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.

Comment 11

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

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.

Comment 12

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

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.

Comment 13

[(not currently active) Ted Mielczarek]

Oh, ugh, this is Universal builds mucking things up.

Comment 14

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

Attachment: Add ability to open support files in gtests - Command line version

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

Comment 15

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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