Closed Bug 780807 Opened 12 years ago Closed 9 years ago

browser and other reporting statistics should not be literal strings in run_tests.py (and possibly elsewhere)

Categories

(Testing :: Talos, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: k0scist, Assigned: bharathkallurs, Mentored)

Details

(Whiteboard: [good first bug][lang=py])

Attachments

(5 files)

In run_tests.py (and possibly elsewhere) we use a bunch of literal
strings of 'NULL' (maybe others):

http://hg.mozilla.org/build/talos/file/f55ac228921e/talos/run_tests.py#l93
http://hg.mozilla.org/build/talos/file/f55ac228921e/talos/run_tests.py#l190

These should be None (the singleton) if they should be defined at all
(questionable).  Output formats, such as graphserver, can format them
to 'NULL' iff such is appropriate, but probably not run_tests.py's job.
Whiteboard: [good first bug][mentor=jhammel][lang=py]
I believe the NULL is needed for the graph server SQL.  If we move it to output that is fine.
i'm looking to get involved with the mozilla community. anyone willing to help me get started on this bug, if not already assigned?
Attachment #654666 - Flags: review?(jhammel)
Comment on attachment 654666 [details] [diff] [review]
hope this is fine :)

So this doesn't really suffice. While you've changed these in run_test.py, they are likely used/referenced in other files, such as ttest.py and output.py and perhaps more.  

I'm also not sure why you alter the test file at all, or why run_tests.pyc is in your diff

+    if browser_config.get('repository', None) == None:

This should be if browser_config.get('repository') is None
Attachment #654666 - Flags: review?(jhammel) → review-
(In reply to Jeff Hammel [:jhammel] from comment #4)
> Comment on attachment 654666 [details] [diff] [review]
> hope this is fine :)
> 
> So this doesn't really suffice. While you've changed these in run_test.py,
> they are likely used/referenced in other files, such as ttest.py and
> output.py and perhaps more.  
> 
> I'm also not sure why you alter the test file at all, or why run_tests.pyc
> is in your diff
> 
> +    if browser_config.get('repository', None) == None:
> 
> This should be if browser_config.get('repository') is None


i pushed the repo again and tried and its
 browser_config.get('repository', None) == None 
itself .. also only run_tests file and output have NULL in them 
and its required to be NULL in output .. hence i can only make changes in run_tests
This bug is still worth having and up to date.  Any thoughts, jdev?
Flags: needinfo?(jdev005)
Hi,

 I would like to work on this bug. Can anybody guide me on how to get started with it? 

Thanks a lot.
Hi Gobelinus,

Do you understand the bug?  Do you understand where to get the talos code?  Maybe telling what you need to know is the most expedient.

Thanks for the help!

Jeff
Hi Jeff,

As per my understanding, we need to replace literal strings with python equivalents like 'NULL' with None, however output should remain same till graphserver uptakes such changes. Otherwise graphserver reports will break.

I cloned talos code. I was just wondering, if there is any way in which I can generate and view reports without graphserver. I want to compare output before and after making these changes.

Thanks
Gobelinus
You should be able to use `--results_url file://$(pwd)/output.txt` to get local results, I think.  If this does not suffice, you'll have to hack the code.
Hi Jeff,

I am keen on working on this bug.
i have the following questions to start with :
1. I need to replace literal strings like 'NULL' to None in all the places occuring in the run_tests.py. Could you please confirm if my understanding is right, else correct me?

2. Where can i clone talos code so that i can start hacking run_tests.py code?

3. Once that is done, where do i submit the diff patch?

Thank you!
Bharath,

I will help you out, here is how to get started with talos:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

Once you can run a test (use 'tsvgx') as it is simple to run, I would make your change and ensure everything works.  When you submit the patch, we use mercurial queues (https://developer.mozilla.org/en-US/docs/Mercurial_Queues) usually, but you can 'hg diff > patch" and attach it to this bug.
Flags: needinfo?(jdev005)
Whiteboard: [good first bug][mentor=jhammel][lang=py] → [good first bug][mentor=jmaher][lang=py]
Joel,

Thanks for the quick reply!
Will start having a look at it right away!
Hi Joel,

I just followed the link that gave above to setup the local buildbot for talos.
I am stuck at run tests phase of the article. i tried executing the command given there and i am facing an issue.
Could you please help me resolve it? searching the forums for it was not really useful for me.
AFAIK i am either giving a wrong format for the command or a flag incomplete.

Please find the console logs as follows :

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
in the commandline you have above, there is "--output ts_des", if you make this "--output ts_des.yml" it will work.  That sounds like another bug in talos.
I would like to work on this.Can someone assign it to me?
hi dweep- it looks like Bharath is working on this- would you like to take another bug: http://www.joshmatthews.net/bugsahoy/?automation=1&py=1 ?
Thanks for clarifying Joel!

So would you recommend me to log a bug for the CLI as well?

I will change the output arg and see how that will work for me..

Thanks!
yes, please file a bug about the cli issue!  Thanks for finding it.
Assignee: nobody → bharathkallurs
Hi Joel,

I just was trying the talos with the modified CLI. I am having an issue in starting the tests on firefox. As i am already running firefox for my personal use and i don't want to switch to aniother browser, the plugins running on my current session of firefox are blocking from starting the tests. I followed a few blogs on internet to start the firefox with a different user profile which still doesn't seem to help. I guess user profiles are just for accessing, firefox as multiple users with their own customizations for each profile. 
Could you please guide/ point me to a link which can help me setup firefox for my dev environment which does not effect my regular usage? more like a virtualenv setup? :-)

Kindly have a look at the logs :

(talos)root@ubuntu:/opt/foxproject/talos# talos -n -d --develop --executablePath "/usr/bin/firefox" --activeTests ts --results_url ts.txt --datazilla-url ts.json --output ts_desktop.yml
 - outputName = ts_desktop.yml
DEBUG : using testdate: 1392049173
DEBUG : actual date: 1392049173
INFO : browserInfo: '/usr/bin/application.ini' does not exist
qm-pxp01: 
		Started Mon, 10 Feb 2014 21:49:33
Running test ts: 
		Started Mon, 10 Feb 2014 21:49:33
DEBUG : operating with platform_type : linux_
DEBUG : firefox already running before testing started (unclean system)
INFO : 
(process:3339): GLib-CRITICAL **: g_slice_set_config: assertion 'sys_page_size == 0' failed

(firefox:3339): GLib-GIO-CRITICAL **: g_dbus_connection_register_object: assertion 'G_IS_DBUS_CONNECTION (connection)' failed
. . . . 

(firefox:3339): GConf-WARNING **: Client failed to connect to the D-BUS daemon:
Did not receive a reply. Possible causes include: the remote application did not send a reply, the message bus security policy blocked the reply, the reply timeout expired, or the network connection was broken.
__start_reportNaN__end_report
__startTimestamp1391709305279__endTimestamp
__startBeforeLaunchTimestamp1391709304000__endBeforeLaunchTimestamp
__startAfterTerminationTimestamp1391709306000__endAfterTerminationTimestamp

Failed ts: 
		Stopped Mon, 10 Feb 2014 21:49:33
Traceback (most recent call last):
  File "/opt/foxproject/talos/talos/run_tests.py", line 329, in run_tests
    talos_results.add(mytest.runTest(browser_config, test))
  File "/opt/foxproject/talos/talos/ttest.py", line 293, in runTest
    raise talosError("Found processes still running: %s. Please close them before running talos." % running_processes_str)
talosError: Found processes still running: [3614] firefox, [3679] plugin-container, [24360] plugin-container. Please close them before running talos.
In order to run talos, you need to terminate all other instances of firefox locally.  It is an annoyance that we have to live with, this is in place for a reason as we need less stuff interfering with the system while running performance tests.

When we launch firefox from talos, we create a temporary profile and use that specifically.  This has a custom set of preferences and extensions.
Mentor: jmaher
Whiteboard: [good first bug][mentor=jmaher][lang=py] → [good first bug][lang=py]
Sorry for getting into hibernate mode on this bug for a long time. I am back at it again and will be working on this. I am currently just retesting my setup and will soon report what i face.
great, feel free to ask questions when you have them!
The diff contains the files which had NULL previously which are now changed to None (the singleton).
Hi Joel,

Kindly review the diff that I posted and please do let me know if there is any change that i have to make.

I could find the following files which had the NULL string instead of None singleton.

(talos)bkallur@bharathkallur:/opt/talos/talos/talos$ grep -rai "Null" *.py
cmanager_win32.py:                # double null: we're done
output.py:                    vals = [[x, 'NULL'] for x in values]
output.py:                        if page == 'NULL':
output.py:            if results.revision and results.revision != 'NULL':
results.py:            result['page'] = 'NULL'

Thanks!
Comment on attachment 8535998 [details] [diff] [review]
NULL_to_None.diff

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

yay, this looks pretty good. Please outline what type of testing you have done and how you have verified this.  Once we get some confidence that this is working, I will push it up to our try server and run the tests like we do in production :)

::: talos/output.py
@@ +536,4 @@
>              else:
>                  params['x86'] = 'false'
>  
> +            if results.revision and results.revision is not None : #changing'NULL' to None

in this case you just need:
if results.revision:
Attachment #8535998 - Flags: review+
ts.txt file which was obtained as result from the change made.
This is the .yml obtained after the change made in the code.
Hi Joel,

Thank you for review comments.. :)
I changed the line as you suggested. Seems more clean now.
I ran the test using the standard command line given on the talos build/running instruction page.
I have attached the results file obtained after the run.

The command line i used to run the tests and verify the change is :
talos -n -d --develop --executablePath /opt/foxproject/firefox/firefox --activeTests ts --results_url ts.txt --datazilla-url ts.json --output ts_desktop.yml --mozAfterPaint

The last line of the result obtained on the console is which is the content of ts.json file is :
INFO : TALOSDATA: [{"test_machine": {"platform": "x86_64", "osversion": "Ubuntu 14.04", "os": "linux", "name": "qm-pxp01"}, "talos_aux": {"shutdown": [275, 680, 959, 65, 500, 918, 60, 80, 314, 562, 744, 88, 274, 235, 361, 748, 104, 397, 829, 236]}, "testrun": {"date": 1418572435, "suite": "ts", "options": {"responsiveness": false, "tpmozafterpaint": true, "tpchrome": true, "tppagecycles": 1, "tpcycles": 10, "tprender": false, "shutdown": true, "extensions": [{"name": "pageloader@mozilla.org"}], "rss": false}}, "results": {"ts": [NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN]}, "test_build": {"version": "34.0.5", "revision": "8274648ad79f", "id": "20141126041045", "branch": "", "name": "Firefox"}}]
TinderboxPrint: TalosResult: {}


It seems to me that the tests are running fine and launching. Could you please have a look at the output files once and let me know if I am missing something to analyse?
Also a point or 2 (or a wiki) which will help me analyse and run specific tests might be really helpful. This will help me understand sort of tests that i should run from talos for the change i make.

Thanks again for all the help! :)
I have pushed this to try server, we will see how this looks.  If all goes well, then we can land this and get it deployed.

Since it looks like you have setup talos and were able to get it running locally with no problem, here is sort of a more difficult bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=776947

If that is too hard or seems a bit complicated, I can find one that isn't so involved.

Thanks again for your work on this bug, we will get it deployed this week if all goes well.
Thanks Joel!

Sure.. I shall take a look at the new bug and try comprehending.

Thanks again..
I ran this on try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=882e7b276e0b

the oranges are errors where we are uploading a page name None instead of NULL and graph server is complaining.  I really don't like having a bug available and then getting a patch/fix to only find out there is not a big need for it.  One way to hack around this would be to try making sure graphserveroutput in output.py keeps NULL when it is defining the dataset.  We could try that out!
Joel,

I just saw your comment. Please let me know if you are referring to the change ?

--- a/talos/output.py	Fri Dec 12 15:38:37 2014 -0500
+++ b/talos/output.py	Sat Dec 13 16:13:16 2014 +0530
@@ -216,7 +216,8 @@
                         continue
 
                     # counter values
-                    vals = [[x, 'NULL'] for x in values]
+                    #changing NULL to None
+                    vals = [[x, None] for x in values]

So restoring the NULL back in vals which is defined in GraphServerOutput class?
I believe that would do it.
Adding a version 2 of patch to improve the result in graphserveroutput. This patch restores the NULL as default values for "vals" variable, in GraphServerOutput class of output.py file. The patch is tested and has the following output

INFO : TALOSDATA: [{"test_machine": {"platform": "x86_64", "osversion": "Ubuntu 14.04", "os": "linux", "name": "qm-pxp01"}, "talos_aux": {"shutdown": [116, 576, 71, 559, 46, 503, 922, 460, 853, 356, 954, 520, 141, 720, 314, 911, 460, 794, 387, 4]}, "testrun": {"date": 1418828448, "suite": "ts", "options": {"responsiveness": false, "tpmozafterpaint": true, "tpchrome": true, "tppagecycles": 1, "tpcycles": 10, "tprender": false, "shutdown": true, "extensions": [{"name": "pageloader@mozilla.org"}], "rss": false}}, "results": {"ts": [NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN]}, "test_build": {"version": "34.0.5", "revision": "8274648ad79f", "id": "20141126041045", "branch": "", "name": "Firefox"}}]
TinderboxPrint: TalosResult: {}
Comment on attachment 8537836 [details] [diff] [review]
NULL_to_None_v2.patch

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

this failed again:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-618fbb19cf54/try-linux/try_ubuntu32_hw_test-chromez-bm104-tests1-linux-build774.txt.gz

I think for the number of instances of null and what we gain from this, we should move onto more productive things.  In the future where we do not deal with graph server, this might make more sense!

So lets move onto some other fun bugs- there are a lot that will help us make talos better and more useful!
Attachment #8537836 - Flags: review-
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: