Closed Bug 784834 Opened 8 years ago Closed 7 years ago

twinopen tests do not work with --develop

Categories

(Testing :: Talos, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: k0scist, Assigned: jmaher)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
"""
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.
Whiteboard: [good first bug][mentor=jhammel][lang=py]
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 :(
Whiteboard: [good first bug][mentor=jhammel][lang=py]
FWIW, this is a firefox internal behaviour: relative paths do not escape e.g. '?' whereas absolute paths do :(
This does however work with file:// URLs :)
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 :/
Attached patch WIP (obsolete) — Splinter Review
This is insufficient but a brief stab at the problem.  Recording for posterity
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:
Does it work if you're not in the same directory as run_tests.py?
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).
I'll take a look at this
Assignee: nobody → jhammel
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
Attached patch this should do it (obsolete) — Splinter Review
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.
Attachment #655043 - Attachment is obsolete: true
Attachment #667151 - Flags: review?(jmaher)
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.
Attachment #667151 - Flags: review?(jmaher) → review+
carrying r=jmaher forward
Attachment #667151 - Attachment is obsolete: true
Attachment #667247 - Flags: review+
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
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/jhammel@mozilla.com-df082165f06d
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.
Assignee: jhammel → jmaher
Attachment #667247 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #667513 - Flags: review?(jhammel)
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
Attachment #667513 - Flags: review?(jhammel) → review-
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/jmaher@mozilla.com-f523d9f02afd
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/jmaher@mozilla.com-f523d9f02afd
it appears my patch causes dromaeojs to fail on windows.
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/jmaher@mozilla.com-f523d9f02afd
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.
It did seem quite unusual that Joel's change would cause that particular failure.
welcome the addition of desktop and mobile tags for tests.
Attachment #667513 - Attachment is obsolete: true
Attachment #668022 - Flags: review?(jhammel)
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.
(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)
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.
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...
(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 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)
Attachment #668022 - Flags: review?(jhammel) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Looks great, :jmaher!  thanks for doing this
Depends on: 801633
You need to log in before you can comment on or make changes to this bug.