Joel Maher ( :jmaher ) (UTC -8) Reporter
	Description • 11 years ago

as mentioned in the review comments from bug 923770, putting generateTalosConfig in utils.py is just throwing random stuff into a file.  We should move this into its own file and document the need for it.

right now this is needed for xperf scripts (maybe there is a cleaner way to generate it or a different way to send the data to the setup/cleanup processes)

a secondary bug or a bonus would be to make xperf use bcontroller.yml and commandline arguments.  Right now there is some data inside of talos (the pid of the process) which is only known at runtime.

	Vikas Mishra [:mishravikas]
	Comment 1 • 11 years ago

hey joel i'd like to work on it as my first bug please let me know how to start
Thanx

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

thanks for the patch.

a few things:
1) hg add <new filename>
2) please make the function name generateTalosConfig (not GenerateTalosConfig)
3) I don't see where you added this to ttest.py
4) we don't need to import it to utils.py

	Vikas Mishra [:mishravikas]
	Comment 4 • 11 years ago

New patch with above mentioned changes

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

Comment on attachment 8339402 [details] [diff] [review]
New patch

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

lets address these items.

I would also like to create a file in tests/, test_talosconfig.py which would pass in a command_line, browser_config, and test_config- you could print those out during a run locally to see what values they hold.  If we recreate those statically in the test_talosconfig.py then call this we could validate the output file.

thanks for the quick turnaround on addressing the feedback!

::: talos/talosconfig.py
@@ +20,5 @@
> +        browser_config['xperf_providers'] = test_config['xperf_providers']
> +        browser_config['xperf_user_providers'] = test_config['xperf_user_providers']
> +        browser_config['xperf_stackwalk'] = test_config['xperf_stackwalk']
> +        browser_config['processID'] = pid
> +        browser_config['approot'] = os.path.dirname(browser_config['browser_path'])

we need an 'import os' at the top of this file to have access to os.path.

@@ +23,5 @@
> +        browser_config['processID'] = pid
> +        browser_config['approot'] = os.path.dirname(browser_config['browser_path'])
> +        bcontroller_vars.extend(['xperf_providers', 'xperf_user_providers', 'xperf_stackwalk', 'processID', 'approot'])
> +
> +    content = writeConfigFile(browser_config, bcontroller_vars)

where is writeConfigFile defined at?

::: talos/ttest.py
@@ +28,3 @@
>  from threading import Thread
>  
> +

please don't add a new blank line here

::: talos/utils.py
@@ +17,4 @@
>  import platform
>  from mozprocess import pid as mozpid
>  
> +

please don't add a new blank line here.

	Vikas Mishra [:mishravikas]
	Comment 6 • 11 years ago

Thanx for such a prompt review, I've made all the changes except making new files in /tests which I'll make in another patch

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

To do the test case, I would do this:
* add a print statement right before setup is called (http://hg.mozilla.org/build/talos/file/tip/talos/ttest.py#l366):
print command_line
print browser_config
print test_config

* run talos and you will see what values we are passing in.
* create tests/test_testconfig.py
** this will have the values that you printed out as 3 local variables
** import talosconfig
** call talosconfig.generateTalosConfig(var1, var2, var3)
** this will generate a file (you might need to adjust browser_config to set the filename)
** open the file up and verify the fields in there match what you have input in

* create a case where there is data which might not be normal (spaces, embedded \n, etc.)
* make sure it runs by running 'python runtests.py'
* bundle this up in a patch!

	Vikas Mishra [:mishravikas]
	Comment 8 • 11 years ago

Am not able to see the output in the command line after adding those 3 print statements btw I added them before line 370 http://hg.mozilla.org/build/talos/file/tip/talos/ttest.py#l370

and when I run 'runtests.py' while passing strings as var1,var2 and var3 it gives me error "expected 'int' " but when i pass int it gives another error "int type objects not iterable"

can you please help me out with this

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

put the print on line 365 (before the if clause):
http://hg.mozilla.org/build/talos/file/tip/talos/ttest.py#l365

you will need to assign the values of the variables to valid values, probably a dictionary:
http://docs.python.org/2/library/stdtypes.html

you can also print the types they are:
print type(var)

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

Vikas- this bug is assigned to you, are you still interested in finishing this?

	Vikas Mishra [:mishravikas]
	Comment 11 • 11 years ago

sorry for the delay I had my exams in my college and now am done with it and I'll start with it immediately

	Vaibhav (:vaibhav1994) Assignee
	Comment 12 • 11 years ago

Can I take up this bug?

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

It has been a month since Vikas replied, I am fine with you taking over this bug.

	Vaibhav (:vaibhav1994) Assignee
	Comment 14 • 10 years ago

This patch essentially moves generateTalosConfig to a new file, and adds unittests for its testing.

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

Comment on attachment 8368749 [details] [diff] [review]
talos_config.patch

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

please fix these little details, upload a new patch and we can land this!

::: talos/talosconfig.py
@@ +8,5 @@
> +        bcontroller_vars.append('xperf_path')
> +        bcontroller_vars.extend(['buildid', 'sourcestamp', 'repository', 'title'])
> +        if 'name' in test_config:
> +          bcontroller_vars.append('testname')
> +          browser_config['testname'] = test_config['name']

nit: please make this 4 space indentation to match the file, not 2 space

::: tests/test_talosconfig.py
@@ +11,5 @@
> +class TalosConfigUnitTest(unittest.TestCase):
> +    """
> +    A class inheriting from unittest.TestCase to test the generateTalosConfig function.
> +    """
> +    

nit: blank line shouldn't have whitespace

@@ +18,5 @@
> +        if var1 == var2:
> +            return 1
> +        else:
> +            print "input '%s' != expected '%s'"%(var1,var2)
> +            

nit: blank line shouldn't have whitespace

@@ +25,5 @@
> +
> +        browser_config_copy = browser_config.copy()
> +        test_config_copy = test_config.copy()
> +        test = talosconfig.generateTalosConfig(command_args, browser_config_copy, test_config_copy)
> +        

nit: blank line shouldn't have whitespace

@@ +55,5 @@
> +        self.validate(content['approot'],"test/path/to")
> +
> +    def test_errors(self):
> +        # Tests if errors are correctly raised.
> +        

nit: blank line shouldn't have whitespace

@@ +63,5 @@
> +        del browser_config_copy['xperf_path']
> +        talosconfig.generateTalosConfig(command_args, browser_config_copy, test_config_copy)
> +        yaml = YAML()
> +        content = yaml.read(browser_config['bcontroller_config'])
> +        

nit: blank line shouldn't have whitespace

@@ +70,5 @@
> +
> +        # Test to see if keyerror is raised or not for calling testname when xperf_path is missing
> +        with self.assertRaises(KeyError):
> +            self.validate(content['testname'],"tp5n")
> +            

nit: blank line shouldn't have whitespace

@@ +98,5 @@
> +
> +        # Test to see if keyerror is raised or not when calling approot when xperf_providers is missing
> +        with self.assertRaises(KeyError):
> +            self.validate(content['approot'],"test/path/to")
> +        

nit: blank line shouldn't have whitespace

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

https://hg.mozilla.org/build/talos/rev/7ac36dc152ef