784834 - twinopen tests do not work with --develop

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

Running:

"""
talos -e `which firefox` --activeTests ts_paint:tpaint --setPref
dom.send_after_paint_to_content=true -n -d --develop
"""

Talos displays:

"""
DEBUG: command line: /home/jhammel/firefox/firefox  -profile
/tmp/tmpIxFBck/profile
startup_test/twinopen/winopen.xul?mozafterpaint=1%26phase1=20
"""

Which opens the url: http://startup_test/twinopen/winopen.xul?mozafterpaint=1%26phase1=20

This causes Firefox to display:

"""
The proxy server is refusing connections
      
      
      
      
      
        
        
          Firefox is configured to use a proxy server that is refusing
          connections.
        

        
        

  Check the proxy settings to make sure that they are correct.
  Contact your network administrator to make sure the proxy server is
    working.
"""

Comment 1

[Jeff Hammel]

So this is because i didn't `cd $(python -c 'import os, talos; print os.path.dirname(os.path.abspath(talos.__file__))')`.  So we should make this use the absolute path of the file, as if the file actually exists then firefox will open it instead of treat it as a URL and fail out.

Comment 2

[Jeff Hammel]

Hmmm....so this turns out to be not as easy as thought :(

firefox startup_test/twinopen/winopen.xul?mozafterpaint=1%26phase1=20

works fine, however

firefox ${PWD}/startup_test/twinopen/winopen.xul?mozafterpaint=1%26phase1=20

displays:

"""
File not found
      
      
      
      
      
        
        
          Firefox can't find the file at /home/jhammel/mozilla/src/talos/src/talos/talos/startup_test/twinopen/winopen.xul?mozafterpaint=1%26phase1=20.
        

        
        

  Check the file name for capitalization or other typing errors.
  Check to see if the file was moved, renamed or deleted.
"""

Not sure why this is or what can be done about it :(

Comment 3

[Jeff Hammel]

FWIW, this is a firefox internal behaviour: relative paths do not escape e.g. '?' whereas absolute paths do :(

Comment 4

[Jeff Hammel]

This does however work with file:// URLs :)

Comment 5

[Jeff Hammel]

So I think the solution is this:

- the tests that require file:// URLs (currently, just XUL, e.g. startup_test/twinopen/winopen.xul?phase1=20) are changed in http://hg.mozilla.org/build/talos/file/432ec68f5b74/talos/test.py to be file:// URLs refering to the ${talos} directory, e.g. file://${talos}/startup_test/twinopen/winopen.xul?phase1=20

- these are then interpolated with interpolatePath: http://hg.mozilla.org/build/talos/file/432ec68f5b74/talos/utils.py#l238

I am not sure if this logic needs to be altered for remotePerfConfigurator. Ideally, one would take all tests with file:// URLs and copy them to the remote device in run_tests.py, vs remotePerfConfigurator: http://hg.mozilla.org/build/talos/file/432ec68f5b74/talos/remotePerfConfigurator.py#l205 . See also bug 735502 . But there are also support files: http://hg.mozilla.org/build/talos/file/432ec68f5b74/talos/remotePerfConfigurator.py#l215 . So it isn't quite that easy :/

Comment 6

[Jeff Hammel]

Attachment: WIP

This is insufficient but a brief stab at the problem.  Recording for posterity

Comment 7

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

This works fine for me:

python PerfConfigurator.py -v -e `which firefox` -a tpaint --mozAfterPaint --develop --output tpaint.yml
.
:
test_name_extension: _paint
test_timeout: 1200
tests:
- name: tpaint
  timeout: 300
  url: startup_test/twinopen/winopen.xul?mozafterpaint=1%26phase1=20
title: qm-pxp01
webserver: localhost:15707

-----------

(talos)jmaher@jmaher-MacBookPro:~/mozilla/talos/talos$ python run_tests.py -d -n tpaint.yml 
setting debug
DEBUG: using testdate: 1348778054
DEBUG: actual date: 1348778054
RETURN:<a href = "NULL/rev/NULL">rev:NULL</a>
qm-pxp01: 
		Started Thu, 27 Sep 2012 16:34:14
Running test tpaint: 
		Started Thu, 27 Sep 2012 16:34:14
DEBUG: operating with platform_type : linux_
DEBUG: created profile
	Screen width/height:1440/900
	colorDepth:24
	Browser inner width/height: 1024/702


browser_name:Firefox
browser_version:15.0.1
buildID:20120907231657

DEBUG: initialized firefox
DEBUG: command line: /usr/bin/firefox  -profile /tmp/tmpkoHjhs/profile startup_test/twinopen/winopen.xul?mozafterpaint=1%26phase1=20
NOISE: __start_report206|168|143|144|195|127|162|150|135|157|141|142|142|130|142|156|150|159|154|224__end_report__startTimestamp1348778111121__endTimestamp
NOISE: openingTimes=168,143,144,195,127,162,150,135,157,141,142,142,130,142,156,150,159,154,224
NOISE: avgOpenTime:154
NOISE: minOpenTime:127
NOISE: maxOpenTime:224
NOISE: medOpenTime:150
NOISE: __xulWinOpenTime:150
NOISE: __startBeforeLaunchTimestamp1348778066109__endBeforeLaunchTimestamp
NOISE: __startAfterTerminationTimestamp1348778111761__endAfterTerminationTimestamp
Completed test tpaint:

Comment 8

[Jeff Hammel]

Does it work if you're not in the same directory as run_tests.py?

Comment 9

[cmtalbert]

So, I was trying to help gavin with this yesterday, and if you follow the instructions here: https://wiki.mozilla.org/Buildbot/Talos#Running_locally_-_Source_Code it fails to find the winopen.xul file from the webserver.

(We both used --develop so it was using mozhttpd).

Comment 10

[Jeff Hammel]

I'll take a look at this

Comment 11

[Jeff Hammel]

Output with the above 

DEBUG: command line: /home/jhammel/firefox/firefox  -profile /tmp/tmplDezjw/profile /home/jhammel/mozilla/src/talos/src/talos/talos/startup_test/twinopen/winopen.xul?mozafterpaint=1%26phase1=20

Comment 12

[Jeff Hammel]

Attachment: this should do it

I'm not overly happy with the way that we handle urls in test.py currently (which results in complexity in their handling everywhere else), but I'll ticket that separately.  This should work for now, I think.

Comment 13

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

Comment on attachment 667151 [details] [diff] [review]
this should do it

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

only concern might be with the remote stuff, but I think it might be just good...some testing and/or try server can validate all of that.

Comment 14

[Jeff Hammel]

Attachment: had to correct for assumption of CWD...can we not assume CWD, plz?

carrying r=jmaher forward

Comment 15

[Jeff Hammel]

tested;  this appears to work locally on remote (hard to tell since remote testing is always a bit flakey for me); pushed to try and hoping for the best: https://tbpl.mozilla.org/?tree=Try&rev=df082165f06d

Comment 16

[Mozilla RelEng Bot]

Try run for df082165f06d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=df082165f06d
Results (out of 69 total builds):
    success: 64
    failure: 5
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-df082165f06d

Comment 17

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

Attachment: fix tpaint to work with develop and non local directories (1.0)

this patch allows us to use the ${talos} directory in tpaint test.py definition.  This doesn't affect robocop based tests and is almost done on try server.

Comment 18

[Jeff Hammel]

Comment on attachment 667513 [details] [diff] [review]
fix tpaint to work with develop and non local directories (1.0)

So this won't work for remote devices.  If we're not supporting this test, then we should err out (probably in PerfConfigurator) if we it is specified for remote devices, perhaps if you try to run anything with .xul.

Note also that if we're not supporting this, https://bugzilla.mozilla.org/show_bug.cgi?id=735502 should be closed and a new bug should be open to remove the considerable logic we do to copy twinopen files over to android which quite frankly I would be happy to get rid of.

Other than that, the approach looks fine.  I initially started down this line but then started thinking about the remote case

Comment 19

[Mozilla RelEng Bot]

Try run for f523d9f02afd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f523d9f02afd
Results (out of 70 total builds):
    exception: 2
    success: 65
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f523d9f02afd

Comment 20

[Mozilla RelEng Bot]

Try run for f523d9f02afd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f523d9f02afd
Results (out of 71 total builds):
    exception: 2
    success: 65
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f523d9f02afd

Comment 21

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

it appears my patch causes dromaeojs to fail on windows.

Comment 22

[Mozilla RelEng Bot]

Try run for f523d9f02afd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f523d9f02afd
Results (out of 77 total builds):
    exception: 2
    success: 71
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f523d9f02afd

Comment 23

[Phil Ringnalda (:philor)]

I don't ordinarily like to blame people for their parentage, but the parent of your try push was backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/809b60046c5b because, among other things, it caused dromaeojs to fail on windows.

Try, try again, and either an m-c rev that's a merge from a clean tree, or at least an inbound rev which has completely finished running tests will probably serve you better.

Comment 24

[Jeff Hammel]

It did seem quite unusual that Joel's change would cause that particular failure.

Comment 25

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

Attachment: fix tpaint to work with develop and non local directories (2.0)

welcome the addition of desktop and mobile tags for tests.

Comment 26

[Jeff Hammel]

My intention here was more of a quick fix.  What I don't want to do is getting into the business of keeping track what is run versus what can be run.  If something can't/won't work on a platform, it is probably good to mark as such.  If something does work but we just don't run it, we should not mark it as such

@@ -49,6 +51,7 @@
     url = 'startup_test/startup_test.html?begin='
     url_timestamp = True
     shutdown = True
+    desktop = False

This works on desktop for me

 class tspaint_places_generated_max(ts_paint):
     profile_path = '${talos}/places_generated_max'
     timeout = None
+    mobile = False

Does this not work on mobile?

 class tresize(TsBase):
     cycles = 20
     url = 'startup_test/tresize-test.html'
     timeout = 150
     filters = [['mean', []]]
+    mobile = False

Does this not work on mobile?

 class tscroll(PageloaderTest):
     tpmanifest = '${talos}/page_load_test/scroll/scroll.manifest'
     tpcycles = 5
+    mobile = False

Does this not work on mobile?

 class a11y(PageloaderTest):
     tpmanifest = '${talos}/page_load_test/a11y/a11y.manifest'
     tpcycles = 5
+    mobile = False

Does this not work on mobile?

 class a11yr(PageloaderTest):
     tpmanifest = '${talos}/page_load_test/a11y/a11y.manifest'
     tpcycles = 1
     tppagecycles = 25
+    mobile = False

Does this not work on mobile?

I'm also very hesistent to discriminate desktop vs mobile like this.  Currently that is the division.  What about all the different platforms?  Linux, mac, windows, android, b2g, etc.

Comment 27

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

(In reply to Jeff Hammel [:jhammel] from comment #26)
> @@ -49,6 +51,7 @@
>      url = 'startup_test/startup_test.html?begin='
>      url_timestamp = True
>      shutdown = True
> +    desktop = False
> 
> This works on desktop for me

correct, but we don't run it there.  If somebody is trying to run a test and they mistype, they could spend hours debugging the wrong thing.

> 
>  class tspaint_places_generated_max(ts_paint):
>      profile_path = '${talos}/places_generated_max'
>      timeout = None
> +    mobile = False
> 
> Does this not work on mobile?

I haven't tested it and I don't think it will due to resources

> 
>  class tresize(TsBase):
>      cycles = 20
>      url = 'startup_test/tresize-test.html'
>      timeout = 150
>      filters = [['mean', []]]
> +    mobile = False

correct

> 
> Does this not work on mobile?
> 
>  class tscroll(PageloaderTest):
>      tpmanifest = '${talos}/page_load_test/scroll/scroll.manifest'
>      tpcycles = 5
> +    mobile = False
> 
> Does this not work on mobile?

correct

> 
>  class a11y(PageloaderTest):
>      tpmanifest = '${talos}/page_load_test/a11y/a11y.manifest'
>      tpcycles = 5
> +    mobile = False
> 
> Does this not work on mobile?
> 
my understanding is it will not work, likewise with a11yr


There is little difference between operating systems vs desktop or mobile.  mobile doesn't have xul and that is the big difference.  

I would rather have our test definitions indicate where they are run, than hacking it into a harness somewhere.  Any better ideas for enforcing tpaint will not attempt to run on remote (even if we do attempt it will fail before or after this patch)

Comment 28

[Jeff Hammel]

I have rather the opposite opinion: talos is software and should support running whatever actually works.  Talos should not keep track of how it is run in production.  To the extent that we already do so is a hack because altering buildbotcustom/buildbot-configs is time-consuming and is not really under the ateam aegis.  I would ultimately like to move all running details out of talos test definition (tpcycles, cycles, filters, etc).

As such, I would prefer to only mark things that don't run things that *really* don't actually run for technical reasons.

Comment 29

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

in that case, lets remove the desktop=False for the ts test.  If we only had a manifest that listed tests and conditions for when they could run or not run...

Comment 30

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #29)
> in that case, lets remove the desktop=False for the ts test.  If we only had
> a manifest that listed tests and conditions for when they could run or not
> run...

could vs. could not is IMHO a good piece of information to add to talos core (tests.py in this case).  "are vs are not run in production", OTOH, should not live in talos core

I'll r+ this on the contingency that desktop=False is removed for ts and just cross my fingers that this will solve more problems than it builds legacy

Comment 31

[Jeff Hammel]

Comment on attachment 668022 [details] [diff] [review]
fix tpaint to work with develop and non local directories (2.0)

r+ if you remove the desktop=False for ts.  I would really like to check all of the unknown cases but I have no time for this and doubt you do either ;)

Also, some comments around here might help:

+++ b/talos/test.py	Thu Oct 04 12:04:48 2012 -0400
@@ -6,6 +6,8 @@
     """abstract base class for a Talos test case"""
     cycles = None # number of cycles
     keys = []
+    desktop = True # test works on desktop
+    mobile = True  # test works on mobile (android)

Comment 32

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

tested everything and put notes as to why it won't work for the ones turned off:
http://hg.mozilla.org/build/talos/rev/e351f3aa6ecc

Comment 33

[Jeff Hammel]

Looks great, :jmaher!  thanks for doing this