832422 - talosError should follow modern Exception practices

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

http://hg.mozilla.org/build/talos/file/d34468dcc1f8/talos/utils.py#l99

the __init__ and __str__ functions of talosError are pretty
pointless.  Exception does this, essentially, automatically:

>>> class talosError(Exception):
...     """what talosError should look like"""
... 
>>> raise talosError("foo")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
__main__.talosError: foo
>>> try:
...     raise talosError("bar")
... except talosError, e:
...     print str(e)
... 
bar
>>> from talos.utils import talosError
>>> raise talosError("foo")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
talos.utils.talosError: 'foo'
>>> try:
...     raise talosError("bar")
... except talosError, e:
...     print str(e)
... 
'bar'

The only difference is that since we `repr(self.msg)` we get some
extra quotes.  We should get rid of the __init__ and __str__ functions
and add a docstring.

The hard part isn't changing the code.  The hard part is finding and
changing the places we refer to talosError.msg (which really isn't
*that* hard).

Comment 1

[William Lachance (:wlach)]

(In reply to Jeff Hammel [:jhammel] from comment #0)
> http://hg.mozilla.org/build/talos/file/d34468dcc1f8/talos/utils.py#l99
> 
> the __init__ and __str__ functions of talosError are pretty
> pointless.  Exception does this, essentially, automatically:
> 
> >>> class talosError(Exception):
> ...     """what talosError should look like"""
> ... 
> >>> raise talosError("foo")
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> __main__.talosError: foo
> >>> try:
> ...     raise talosError("bar")
> ... except talosError, e:
> ...     print str(e)
> ... 
> bar
> >>> from talos.utils import talosError
> >>> raise talosError("foo")
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> talos.utils.talosError: 'foo'
> >>> try:
> ...     raise talosError("bar")
> ... except talosError, e:
> ...     print str(e)
> ... 
> 'bar'
> 
> The only difference is that since we `repr(self.msg)` we get some
> extra quotes.  We should get rid of the __init__ and __str__ functions
> and add a docstring.
> 
> The hard part isn't changing the code.  The hard part is finding and
> changing the places we refer to talosError.msg (which really isn't
> *that* hard).

You could use facebook's codemod to do this (and why not change the name to TalosError while you're at it):

http://wrla.ch/blog/2012/06/mass-code-relicensing-with-facebooks-codemod/

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

so what should talosError look like?  it would be nice to turn this into an actionable bug, possibly a good first bug !

Comment 3

[Jeff Hammel]

Pretty much what will says in comment 1:

class TalosError(Exception):
    """talos-specific error or better docstring"""

Comment 4

[Pankaj Malhotra (:bitgeeky)]

I am interested in fixing this bug this will be my first bug related to Talos and Hg , please guide how to get started.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

:bitgeeky, great to work with you.

Here is the documentation on talos: https://wiki.mozilla.org/Buildbot/Talos

You can 'hg clone http://hg.mozilla.org/build/talos' to get the source. setup the virtualenv (python INSTALL.py) and try running a test or two. 

Then you can change the code as mentioned in comment #1 to be a real exception/error instead of a bunc hof duplicated and wasted code :)

There are 5 or 6 other really good bugs in Talos that could use some fresh eyes.

Comment 6

[William Lachance (:wlach)]

Filed bug 937210 to deal with talosError vs. TalosError, which we should deal with separately.

Comment 7

[Pankaj Malhotra (:bitgeeky)]

:jmaher , may be we were proceeding in wrong direction , as :jhammel mentioned in bug Description talosError.msg has a significant role to play in this bug.
we must change the files like http://hg.mozilla.org/build/talos/file/0987e4cbd219/talos/output.py#l323 
in which e.msg should be replaced with just e , and also regarding talosError and TalosError 
:wlach filed a bug as in comment #6 ,
Now the scope of this bug reduces to remove the extra __init__ and __str__ , add a docstring , change the files where the talosError.msg is used .
Also I think for this bug we can stick to talosError , and later we can also fix the bug 937210 , as a whole since it will just require running command like : find /path/to/start/from/ -type f | xargs perl -pi -e 's/talosError/TalosError/g'  on the talos directory 
as in : http://www.spiration.co.uk/post/522/in-place-replacement-from-command-line 
if I am wrong anywhere please correct me else we can proceed working on it
Thanks :)

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

I believe there is one variable named talosError_tb, so if you search and replace, make sure it is only on the class name.

the scope of this looks accurate, I look forward to a patch!

Comment 9

[Pankaj Malhotra (:bitgeeky)]

Attachment: 832422.patch

:jmaher please have a look at the patch and suggest changes if any.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 830820 [details] [diff] [review]
832422.patch

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

overall this looks great.  I would like to sanity check it before working on landing it.  Have you simulated some error conditions?  Can we add stuff to the tests/ subfolder to test a few conditions here.

Comment 11

[William Lachance (:wlach)]

Comment on attachment 830820 [details] [diff] [review]
832422.patch

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

Some drive-by comments that IMO should also be addressed.

::: talos/results.py
@@ +91,4 @@
>                  _output.output('file://%s' % os.path.join(os.getcwd(), 'results.out'), results)
>              except:
>                  pass
> +            print '\nFAIL: %s' % e.replace('\n', '\nRETURN:')

This won't work, as e is not a string (it can just be cast to one). Try: str(e).replace('\n', '\nRETURN:')

::: talos/utils.py
@@ +70,1 @@
>  

You should make the same modifications to talosCrash, which was added since this bug was filed (but has the same problem we're talking about)

Comment 12

[Pankaj Malhotra (:bitgeeky)]

Attachment: 832422v1.patch

:wlach please have a look at the patch and suggest changes if any

Comment 13

[William Lachance (:wlach)]

Comment on attachment 831856 [details] [diff] [review]
832422v1.patch

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

Looks good except for some minor problems in test_browser_output.

In normal cases, I would say that you should test your changes before submitting a patch, but it looks like the talos unit tests are currently broken (bug 938664) 
so I can hardly fault you for that. :)

Anyway, everything seems to be working fine after correcting these issues. So please submit a new patch and I'll happily apply.

::: tests/test_browser_output.py
@@ +162,4 @@
>                  import pdb; pdb.set_trace()
> +            self.assertTrue(substr in e)
> +
> +class TestTalosError:

This needs to inherit from unittest.TestCase, otherwise it won't get run. So "class TestTalosError(unittest.TestCase):"

@@ +162,5 @@
>                  import pdb; pdb.set_trace()
> +            self.assertTrue(substr in e)
> +
> +class TestTalosError:
> +    

Please remove this extra line, whitespace we don't want :)

@@ +166,5 @@
> +    
> +    """
> +    test talosError class
> +    """
> +    def test_browser_log_results:

You need to add a self parameter here, so:
 def test_browser_log_results(self):

Comment 14

[Pankaj Malhotra (:bitgeeky)]

Attachment: 832422v2.patch

:wlach thanks for pointing out , made the changes , please have a look again :)

Comment 15

[William Lachance (:wlach)]

Comment on attachment 832375 [details] [diff] [review]
832422v2.patch

Great thanks.

Comment 16

[William Lachance (:wlach)]

Pushed: https://hg.mozilla.org/build/talos/rev/55fd35f46f08