Closed Bug 1401226 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Crash Reporting, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Bug 1396098 demonstrates why throwing away the errors is a a bad idea.
Attachment #8909834 - Flags: review?(ted)
Blocks: 1396098
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.)
Attachment #8909913 - Flags: review?(ted)
Attachment #8909834 - Attachment is obsolete: true
Attachment #8909834 - Flags: review?(ted)
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.
All right, so this is a bit ugly, but it does pass tests everywhere.
Attachment #8910411 - Flags: review?(ted)
Attachment #8909913 - Attachment is obsolete: true
Attachment #8909913 - Flags: review?(ted)
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.
Attachment #8910411 - Flags: review?(ted) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/749ec70a3d40
don't throw away errors from running `file` in symbolstore.py; r=ted.mielczarek
https://hg.mozilla.org/mozilla-central/rev/749ec70a3d40
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.