Closed Bug 1217144 Opened 5 years ago Closed 5 years ago

Running cppunittest locally is broken on OSX

Categories

(Testing :: Mochitest, defect)

All
macOS
defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: spohl, Assigned: spohl)

References

Details

Attachments

(1 file, 3 obsolete files)

Split out from bug 1216575:

(In reply to Eric Rahm [:erahm] from bug 1216575 comment 16)
> STR:
> On a mac (running 10.10.5):
> 1) |mach build|
> 2) |mach cppunittest $OBJ_DIR/dist/bin/TestHashtables|
> 
> There's an exception about the xre_path not being a directory:
> > Exception: ('xre_path does not exist: %s', u'/Users/ericrahm/dev/mozilla-central/obj-x86_64-apple-darwin14.5.0/dist/Resources')
> 
> ahal pointed out this is b/c it's hardcoded in cppunittest [1] to append
> 'Resources' to the path.
> 
> By pointing to the proper app bundle we made cppunitests happy (Resources
> exists) but I guess made everyone else sad. Removing that line seems to work
> for me, but I can't guarantee it won't break other tests.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 605de27d4e7f530159842649c02075c578d7a4a5/testing/runcppunittests.py#235
OS: Unspecified → Mac OS X
Hardware: Unspecified → All
Summary: Running cppunittest locally is broken → Running cppunittest locally is broken on OSX
This if statement was added in bug 1064952. Per bug 1064952 comment 6, running |mach cppunittest| locally used to work. Not sure what changed since then. I don't believe removing the if statement is the right way to go since AFAIK, cppunittest is still run out of the .app bundle on our test slaves.
Attached patch Patch (obsolete) — Splinter Review
Here is a defensive approach. This seems to work for me locally and I sent it to try to confirm there as well (https://treeherder.mozilla.org/#/jobs?repo=try&revision=488428e7e2fb). Eric, could you try this out on your end?
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Flags: needinfo?(erahm)
This patch works for me locally, but it seems a little odd (like that path should never exist given how bindir is set). How are we running the tests on try? I guess not through mach...
Flags: needinfo?(erahm)
My recollection is that xre-path is passed in via cmd line args on our test slaves. We run the tests out of the application bundle. xre-path defaults to bindir, which is Contents/MacOS on OSX and we then modify it to point to Contents/Resources, since that's where the tests are located. This may or may not have changed since I last touched it. I'll submit another try run with the if statement removed completely.
Attached patch Patch with if statement removed (obsolete) — Splinter Review
Hmm, |hg rollback| no longer seems to uncommit the last change after pushing to try, so my previous patch was accidentally based on the first patch in this bug. This patch corrects this. Sent to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb74b6d703a3
Attachment #8677090 - Attachment is obsolete: true
Yeah it appears to be calling runcppunittests.py directly:

> /builds/slave/test/build/venv/bin/python -u /builds/slave/test/build/tests/cppunittest/runcppunittests.py --symbols-path=https://queue.taskcluster.net/v1/task/1cfo4BnBS_if0R0uxytItg/artifacts/public/build/firefox-44.0a1.en-US.mac.crashreporter-symbols.zip --xre-path=/builds/slave/test/build/application/Nightly.app/Contents/MacOS tests/cppunittest

Arguably whatever creates that command should just use the right path, but if that's too hard to figure out your workaround seems reasonable.
Attached patch PatchSplinter Review
This removes the if statement in runcppunittests.py and adds a fix for the mozharness. Waiting for try results (comment 11).
Attachment #8677051 - Attachment is obsolete: true
Attachment #8677096 - Attachment is obsolete: true
Comment on attachment 8677471 [details] [diff] [review]
Patch

Try is green. Eric, if you could make sure that all your tests run locally and give this a review, I'd appreciate it. Thanks!
Attachment #8677471 - Flags: review?(erahm)
Comment on attachment 8677471 [details] [diff] [review]
Patch

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

Sorry for the delay, this definitely seems like the right approach. Applied locally and worked fine for me.
Attachment #8677471 - Flags: review?(erahm) → review+
https://hg.mozilla.org/mozilla-central/rev/5a079ae0c950
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.