Bharath S Kallur Assignee
	Description • 10 years ago

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141126041045

Steps to reproduce:

I started off with mozilla thinking to contribute to the talos test framework. Once i set up the framework and tried to run it on my ubuntu machine, i hit an exception asking to specify output format.


Actual results:

Exception: Please specify a format
(talos)root@ubuntu:/opt/foxproject/talos# talos -n -d --develop --print-tests --executablePath /opt/foxproject/firefox_dev/firefox --activeTests ts --results_url ts.txt --datazilla-url ts.json --output ts_des
 - outputName = ts_des
Traceback (most recent call last):
  File "/opt/foxproject/talos/bin/talos", line 9, in <module>
    load_entry_point('talos==0.0', 'console_scripts', 'talos')()
  File "/opt/foxproject/talos/talos/run_tests.py", line 382, in main
    options, args = parser.parse_args(args)
  File "/opt/foxproject/talos/talos/PerfConfigurator.py", line 554, in parse_args
    options, args = Configuration.parse_args(self, *args, **kwargs)
  File "/opt/foxproject/talos/talos/configuration.py", line 482, in parse_args
    self.dump(options, missingvalues)
  File "/opt/foxproject/talos/talos/PerfConfigurator.py", line 610, in dump
    Configuration.dump(self, options, missingvalues)
  File "/opt/foxproject/talos/talos/configuration.py", line 499, in dump
    self.serialize(dump)
  File "/opt/foxproject/talos/talos/configuration.py", line 533, in serialize
    raise Exception('Please specify a format')
Exception: Please specify a format


Expected results:

either except arg for output as "tes_des" without the extension "yml" or the exception should suggest including the "yml" to the output format argument name.

	:Gijs (he/him)
	Comment 1 • 10 years ago

Apologies for the quietness here! Let me move this bug and needinfo Joel, maybe he can help. :-)

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 2 • 10 years ago

Bharath,

Thanks for filing this bug.  I am happy to help you out here.  So I agree that we should default to a .yml extension (that is the only output format we support).  So I would say please submit a patch in configuration.py for catching this case and adding a .yml extension.  Make sure to output to the end user that you have changed the name on them as well :)

I am in irc on #developers, #ateam, and #perf all the time with the irc nick jmaher, ping me on irc if you want real time answers or just ask in the bug!

	Bharath S Kallur Assignee
	Comment 3 • 10 years ago

Patch to catch the missing output file extension format and if not set, set it to default .yml

	Bharath S Kallur Assignee
	Comment 4 • 10 years ago

Joel,

I just wrote this quick patch for catching the error and resetting the file name and format of it to "yml" if not specified. Please take a look at this and let me know if this fine. :)
I made a run using the above CLI that i have used to log the bug and the tests proceeded further.
Please let me know if you happen to face any issues :)

Thanks!

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 5 • 10 years ago

Comment on attachment 8547575 [details]
YML_EXTN_EXCEPTION.patch

Review of attachment 8547575 [details]:
-----------------------------------------------------------------

Bharath- Thanks for looking into this!  I think you could simplify the patch as outlined below.  Also something to keep in mind is double check your changes to ensure that you don't create a blank line with spaces or tabs (as seen in the patch) and also make there are no trailing whitespace bits at the end of lines you are changing.

::: talos/configuration.py
@@ +522,5 @@
>  
>          if not format:
>              format = self.filename2format(filename)
> +            try :
> +                if not format:

I think this could be simplified to be:
if not format:
    filename += '.yml'
    format = self.filename2format(filename)

	Bharath S Kallur Assignee
	Comment 6 • 10 years ago

Joel, Please check this attachment and let me know if this is ok? :)
I removed the try catch exception block and just kept it simple with the if condition.
Now there is an initial check for format and if that is not present, we add the "yml" extension to the filename and again fetch the format as you suggested.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 7 • 10 years ago

Comment on attachment 8547594 [details] [diff] [review]
YML_EXTN_EXCEPTION_v2.patch

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

really close, just some minor detail cleanup as outlined below.

Another thing, please when you upload mark the other attachments as obsolete, that helps us keep track of what is the current version.  Lastly, make sure you name is on the patch and you add a comment, the comment is usually "Bug XXXXXXX - Title. r=<reviewer".  These little details go a long way!

::: talos/configuration.py
@@ +522,3 @@
>  
>          if not format:
> +			filename += ".yml"

nit: we use 4 space indentation in this file, this looks to be more like 8 (or maybe some tabs)?

@@ +525,2 @@
>              format = self.filename2format(filename)
> +                

nit: this blank line has whitespace in it, how about we just remove this blank line.

	Bharath S Kallur Assignee
	Comment 8 • 10 years ago

Bug 1110604 - CLI to run talos with parameter output and argument "ts_des" is failing with an exception to specify right output format
Fix By : Bharath Kallur
Reviewer : Joel Maher

	Bharath S Kallur Assignee
	Comment 9 • 10 years ago

Hi Joel,

Thank you so much for detailing :) I am sure it will help me in the long run.
I have made the suggested modifications to the patch now. Please have a look at it now.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 10 • 10 years ago

Comment on attachment 8547631 [details] [diff] [review]
YML_EXTN_EXCEPTION_v3.patch

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

Thanks for this patch.  When I mentioned the bug name/comment, this should be in the commit message for the patch, same with setting up hg to have your username:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

I will land the patch, and your next patch can include the commit message!

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 11 • 10 years ago

https://hg.mozilla.org/build/talos/rev/0c60cd971f94

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 12 • 10 years ago

Thanks Bharath!  If you have more time and want to dig into more bugs, we have plenty of bugs you can find online:
http://www.joshmatthews.net/bugsahoy/

Likewise if there is an automation related project that you would like to work on more (including Talos), we have a list of them here:
https://wiki.mozilla.org/Auto-tools/Projects/Everything#Project_Table

Looking forward to working with you more!