1401226 - don't throw away errors from running `file` in symbolstore.py

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[Nathan Froyd [:froydnj]]

We want symbolstore.py to fail, preferably loudly, if we can't find the
necessary tools, and throwing away errors here runs counter to that
goal.  Dumper is a base class for Dumper_Win32, where we probably don't
have file(1), but Dumper_Win32 shouldn't be calling RunFileCommand.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: don't throw away errors from running `file` in symbolstore.py

Bug 1396098 demonstrates why throwing away the errors is a a bad idea.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: don't throw away errors from running `file` in symbolstore.py

Here's a version that doesn't fail tests.  Apparently in the symbolstore tests,
os.popen never got mocked, so we were *actually* calling `file`.  Replacing the
popen call with read_output, which calls subprocess.Popen.communicate() causes
issues, because `read_output` was never called before...meaning that we'd have
to mock .communicate() and that seemed beyond my skills at this point.  (I
tried mocking .communicate() and ran into peculiar errors.)

Comment 3

[Nathan Froyd [:froydnj]]

Oh, I was failing to add in mocked outputs for the `file` calls at appropriate points.  I don't know if redoing those tests would be worth it.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: don't throw away errors from running `file` in symbolstore.py

All right, so this is a bit ugly, but it does pass tests everywhere.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 8910411 [details] [diff] [review]
don't throw away errors from running `file` in symbolstore.py

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

Thanks! Sorry that the mock usage in the tests was such a PITA to get right.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/749ec70a3d40
don't throw away errors from running `file` in symbolstore.py; r=ted.mielczarek

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/749ec70a3d40