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

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: Adds option to send crash reports via email
......................................................................


Patch Set 3: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/#/c/1539/3/lib/python/mailer.py
File lib/python/mailer.py:

Line 2: '''Simple email via SMTP
Use a consistent commenting style for docstrings, either ''' or """.

Generally, we use """, although older code uses '''. At the very least, it should be consistent within a single file.


Line 14: 
This should be prefaced with:

Keyword Arguments:


Line 27:     try:
       :         import smtplib
       :     except ImportError:
       :         print "Failed to import smtplib"
       :         return -1
I'd put the import of smtplib within a try/except catch at the top of the file. If the exception is triggered, set a global variable that smtplib is not available.

If this function is called, print out that it is not available and return.


Line 31:         return -1
If this returns things, it should be documented.

That being said, I don't think it needs to return something. This isn't C.

Since this is a utility function; I'd just let it raise exceptions and let the callers of the function handle the exceptions themselves.


Line 33:     print "Derpsti"
I don't know what this is.


Line 35:     if message['subject']:
If 'subject' is not in message this will crash.

Instead, use:

subject = message.get('subject')
if subject:
    email_text = ...


Line 41:     print "Shooping this whoop"
Remove more debug.


Line 50:     except smtplib.SMTPException as smtp_exception:
       :         print "Failed to send crash report due to unhandled SMTPException"
       :         raise(smtp_exception)
I don't think we ever want an exception here to blow up the entire test suite. This should print out the errors and return.


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

Line 731:     parser.add_option("--email-on-crash", action="store_true",
        :                       dest="email_on_crash", default=False,
        :                       help="Send email on crash with a backtrace. See "
        :                            "crash-mail-config.yaml.sample for details.")
I just alphabetized these. Please place this after -V, before --number.


-- 
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: 3
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Jonathan Rose <jrose at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Jonathan Rose <jrose at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes