937210 - Some Talos class names are really weird

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[William Lachance (:wlach)]

For some reason in talos, many of the class names are not capitalized, so we have:

talosError
talosRegression
talosCrash
talosProcess

This is a really strange convention and is not consistent with either PEP8 or the rest of the talos codebase. Unless I'm missing something IMO we should capitalize these class names. We could use facebook's codemod for this to make sure everything's covered:

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

Comment 1

[William Lachance (:wlach)]

Does the above make sense Joel? Or is there some rationalization behind the above naming convention that I'm unaware of?

Comment 2

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

I am fine with capitalizing the names.  We just need to make sure what is printed out matches the regex stuff that tbpl uses (https://hg.mozilla.org/webtools/tbpl/file/tip/php/inc/ ..)

we can change tbpl, but it is more work.

:edmorley, where does tbpl parse for talos errors?

Comment 3

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #2)
> I am fine with capitalizing the names.  We just need to make sure what is
> printed out matches the regex stuff that tbpl uses
> (https://hg.mozilla.org/webtools/tbpl/file/tip/php/inc/ ..)
> 
> we can change tbpl, but it is more work.
> 
> :edmorley, where does tbpl parse for talos errors?

A quick grep of the tbpl source tree (http://hg.mozilla.org/webtools/tbpl) does not find any reference to any of the class names mentioned above.

Comment 4

[Ed Morley [:emorley]]

(In reply to Joel Maher (:jmaher) from comment #2)
> :edmorley, where does tbpl parse for talos errors?

https://hg.mozilla.org/webtools/tbpl/file/fb30dd66567f/php/inc/GeneralErrorFilter.php#l47

>  47        || preg_match("/^[A-Za-z]+Error:/", $line)  // . . . . . . . . . .  . . Python exceptions

(And yeah very hacky)

So won't be affected :-)

Comment 5

[Sambuddha Basu [:sambuddhabasu1]]

I am interested in solving this bug. Please assign it to me.

Comment 6

[Pankaj Malhotra (:bitgeeky)]

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
might be useful , or facebook codemod http://wrla.ch/blog/2012/06/mass-code-relicensing-with-facebooks-codemod/ 
as :wlach said in Descrition to be sure everything is covered

Comment 7

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Talos class names were changed

Comment 8

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

Comment on attachment 8374915 [details] [diff] [review]
Talos class names were changed

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

all the xtalosError stuff is changed to xTalosError, we should either:
1) leave it alone
2) change it to XTalosError

note: I only commented on lines for a specific file, there are a couple other files with xTalosError in them.

::: talos/xtalos/etlparser.py
@@ +418,5 @@
>      args = xtalos.options_from_config(args, config_file)
>  
>      # ensure process ID is given
>      if not args.get('processID'):
> +        raise xtalos.xTalosError("No process ID option given")

we don't need to change this

@@ +423,4 @@
>  
>      # ensure path to xperf given
>      if not os.path.exists(args['xperf_path']):
> +        raise xtalos.xTalosError("ERROR: xperf_path '%s' does not exist" % args['xperf_path'])

nor this.

Comment 9

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Talos class names were changed

xtalosError was left as it is. The class names mentioned were changed.

Comment 10

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

Comment on attachment 8374955 [details] [diff] [review]
Talos class names were changed

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

this looks good, let me test it on try server.

Comment 11

[William Lachance (:wlach)]

Well, to be consistent with PEP8 and the intent of this bug, we should also change XTalosError to XTalosError

Comment 12

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: Change talks class names

Change talosProcess.py to TalosProcess.py . Also, as mentioned by :wlach, it has been changed to XTalosError

Comment 13

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

Comment on attachment 8375464 [details] [diff] [review]
Change talks class names

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

thanks, this is looking good.  I am running on try to ensure this doesn't cause any random regressions

Comment 14

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

https://hg.mozilla.org/build/talos/rev/a87b48746f70

this will be picked up in the next talos deployment to inbound, probably late next week.

Comment 15

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

oh, this went live in inbound this morning.