[Asterisk-code-review] Add options to send crash reports via email (testsuite[master])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: Add options to send crash reports via email
......................................................................


Patch Set 1: Code-Review-1

(6 comments)

https://gerrit.asterisk.org/#/c/1539/1/runtests.py
File runtests.py:

Line 201:             dest_file = open(dest_file_name, 'r')
        :             email_contents = "{0}\n\n{1}".format(dest_file_name,
        :                                                  dest_file.read())
        :             dest_file.close()
Use 'with' to auto-close the file:

with open(dest_file_name, 'r') as dest_file:
  ....


Line 211:             email_message = "{0}\n\n{1}".format(
        :                 "Subject: {0}".format(subject),
        :                 email_contents)
Your column width in Python is 80 characters. You should be able to format this without this much indentation. Since this is also just string concatenation, you can write this much simpler:

subject_txt = "Subject: " + subject
email_message = subject_txt + "\n\n" + email_contents


Line 219:         except:
A full catch-all except is a bad pattern to fall into.
(a) Some exceptions probably should be thrown
(b) It masks what the exception actually was, which means people can't solve it.

What's more, if the entire routine should be wrapped up in a try/except, the invoker of the function should do that.


Line 725:     parser.add_option("--email-server",
        :                       dest="email_server",
        :                       default=None,
        :                       action="store",
        :                       help=("Specify an email server to use for sending crash"
        :                             "reports"))
Format the options matching the style of this file.


Line 729:                       help=("Specify an email server to use for sending crash"
        :                             "reports")
You don't need () around your help statements.


Line 725:     parser.add_option("--email-server",
        :                       dest="email_server",
        :                       default=None,
        :                       action="store",
        :                       help=("Specify an email server to use for sending crash"
        :                             "reports"))
        :     parser.add_option("--email-sender",
        :                       dest="email_sender",
        :                       default=None,
        :                       action="store",
        :                       help=("Specify email address to act as the sender for "
        :                             "crash reports."))
        :     parser.add_option("--email-recipient",
        :                       dest="email_recipients",
        :                       default=[],
        :                       action="append",
        :                       help=("Add email address to the list of recipents for "
        :                             "crash reports."))
        :     parser.add_option("--email-bt-len",
        :                       dest="email_bt_len",
        :                       default=8000,
        :                       type="int",
        :                       action="store",
        :                       help=("Specify a maximum length for the backtrace "
        :                             "included with crash report emails. Results will "
        :                             "be truncated beyond the limit. 0 for no limit, "
        :                             "defaults to 8000."))
I dislike having four parameters that are required in order to enable what is essentially a single option.

These parameters feel like they should be provided by some configuration file (such as the test-config.yaml file), and read in if a single command line parameter instructs runtests.py to e-mail a backtrace.


-- 
To view, visit https://gerrit.asterisk.org/1539
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I17def41f15294a48754269d9c42aa649389840ad
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Jonathan Rose <jrose at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes