Closed Bug 1355934 Opened 7 years ago Closed 7 years ago

Rename tpaint and move it out of the 'startup_test' directory

Categories

(Testing :: Talos, enhancement)

Product:
Testing
Component:
Talos
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jwatt, Assigned: igoldan)

Details

(Whiteboard: [PI:July])

Attachments

(1 file, 1 obsolete file)

Bug 1355934 - transform tpaint into a Pageloader test
59 bytes, text/x-review-board-request
jmaher
: review+
Details 
tpaint is a test that measures how long it takes to open a new popup window. Having tpaint under the 'startup_test' directory and putting it under the Startup Tests section of the wiki is misleading and likely to lead to confusion. This test should really be renamed to tpopup, or something, and belong to a different category.

https://wiki.mozilla.org/Buildbot/Talos/Tests#Startup_Tests

https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/startup_test/tpaint.html
:igoldan, this might be interesting for you to consider.  I agree this isn't a startup_test and we could structure this to load via pageloader- I would want to make sure we get the same numbers out when running via pageloader and that it is reliable.
Flags: needinfo?(ionut.goldan)
Assignee: nobody → ionut.goldan
Status: NEW → ASSIGNED
Flags: needinfo?(ionut.goldan)
Depends on: 1357512
No longer depends on: 1357512
Comment on attachment 8861884 [details]
Bug 1355934 - transform tpaint into a Pageloader test

https://reviewboard.mozilla.org/r/133900/#review136802

pretty close!

::: testing/talos/talos/test.py:233
(Diff revision 1)
> +    results reported are the average amount of time required to create and
> +    display a window in the running instance of the browser.
> +    (Measures ctrl-n performance.)
> +    """
> +    tpmanifest = '${talos}/tests/tpaint/tpaint.manifest'
> +    tpcycles = 5

we have 20 cycles, please adjust this.

::: testing/talos/talos/tests/tpaint/tpaint.manifest:1
(Diff revision 1)
> +% http://localhost/pageloader/tpaint.html?auto=1

we need the tpaint.html in the same directory as tpaint, not a new pageloader directory.
Attachment #8861884 - Flags: review?(jmaher) → review-
Comment on attachment 8862310 [details]
Bug 1355934 - increase test cycles to 20

https://reviewboard.mozilla.org/r/134240/#review137224

it would be nicer to update the original commit instead of adding a new review- I believe using the same commit message would update mozreview properly and then we could do interdiff to see the incremental changes as we wish.

Getting closer and sorry for misleading you yesterday- the file move is great!

::: testing/talos/talos/test.py:233
(Diff revision 1)
>      results reported are the average amount of time required to create and
>      display a window in the running instance of the browser.
>      (Measures ctrl-n performance.)
>      """
>      tpmanifest = '${talos}/tests/tpaint/tpaint.manifest'
> -    tpcycles = 5
> +    tpcycles = 20

I was wrong about this, we have the 20 cycles defined inside the tpaint.html itself, so we need to either remove that and include it here as you did (ideally what I would like), or make |tpcycles = 1 # we do 20 iterations inside the test|

Lastly, inside of tpaint.html you need to post the results properly- have you tested this and does it work locally?  pageloader tests have a different method for reporting results that startup_tests do using tpRecordTime:
https://dxr.mozilla.org/mozilla-central/search?q=tpRecordTime&redirect=true
Attachment #8862310 - Flags: review?(jmaher) → review-
(In reply to Joel Maher ( :jmaher) from comment #5)

> it would be nicer to update the original commit instead of adding a new
> review- I believe using the same commit message would update mozreview
> properly and then we could do interdiff to see the incremental changes as we
> wish.

I wasn't aware of this thing. I followed the MozReview docs, which indicated to use the exact same steps when a review requires new commits.

> ::: testing/talos/talos/test.py:233
> (Diff revision 1)
> >      results reported are the average amount of time required to create and
> >      display a window in the running instance of the browser.
> >      (Measures ctrl-n performance.)
> >      """
> >      tpmanifest = '${talos}/tests/tpaint/tpaint.manifest'
> > -    tpcycles = 5
> > +    tpcycles = 20
> 
> I was wrong about this, we have the 20 cycles defined inside the tpaint.html
> itself, so we need to either remove that and include it here as you did
> (ideally what I would like), or make |tpcycles = 1 # we do 20 iterations
> inside the test|

For using the first approach, I need to investigate the code more, as I am not familiar with the way JavaScript conducts the tests. I noticed the libraries included in tpaint.html; I need to go through them, plus document on the APIs they are calling. Though a little time consuming, it will help me on the long run, as I will need to know these for future tasks.

> Lastly, inside of tpaint.html you need to post the results properly- have
> you tested this and does it work locally?  pageloader tests have a different
> method for reporting results that startup_tests do using tpRecordTime:
> https://dxr.mozilla.org/mozilla-central/search?q=tpRecordTime&redirect=true

This is something I wanted to ask you about, as I didn't yet figure out were the results are stored locally.
After I made the changes, I compared the way tests ran on startup with the way they run now, by monitoring the console logs. They looked similar. I then pushed the commit on MozReview, to be sure I'm on the right track with the code changes.
Will come back with a response as soon as I finish this test.
(In reply to Joel Maher ( :jmaher) from comment #5)
> Lastly, inside of tpaint.html you need to post the results properly- have
> you tested this and does it work locally?  pageloader tests have a different
> method for reporting results that startup_tests do using tpRecordTime:
> https://dxr.mozilla.org/mozilla-central/search?q=tpRecordTime&redirect=true

I ran tpaint test 3 times for old startup version, and another 3 times for new pageloader version.
The results are as follows:

Previous values:
avgOpenTime | minOpenTime | maxOpenTime | medOpenTime | __xulWinOpenTime 
213.41      | 189.57      | 252.66      | 205.92      | 205.92           
222.87      | 202.41      | 288.27      | 218.0625    | 218.0625         
226.22      | 204.81      | 287.41      | 221.5925    | 221.5925
[AVERAGE] --------------------------------------------------------------
220.8333333 | 198.93      | 276.1133333 | 215.1916667 | 215.1916667   

New values:
avgOpenTime(new) | minOpenTime(new) | maxOpenTime(new) | medOpenTime(new) | __xulWinOpenTime(new)
219.18           | 191.33           | 273.06           | 218.09           | 218.09               
230.6            | 201.72           | 299.38           | 222.555          | 222.555              
229.69           | 203.52           | 288.78           | 211.92           | 211.92               
[AVERAGE] ----------------------------------------------------------------------------------------
226.49           | 198.8566667      | 287.0733333      | 217.5216667      | 217.5216667          
[NEW_AVG - OLD_AVG] --------------------------------------------------------------------------------------
5.656666667      | -0.07333333333   | 10.96            | 2.33             | 2.33
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #6)
> (In reply to Joel Maher ( :jmaher) from comment #5)
> 
> > it would be nicer to update the original commit instead of adding a new
> > review- I believe using the same commit message would update mozreview
> > properly and then we could do interdiff to see the incremental changes as we
> > wish.

Hey Ionut, FYI as I mentioned on IRC, you can use 'hg commit --amend' and then push that to mozreview when submitting more changes to an existing patch that is under review - then it will update the existing commit in MozReview. :)
Attachment #8861884 - Flags: review?(rwood)
Comment on attachment 8862310 [details]
Bug 1355934 - increase test cycles to 20

Hi Ionut, it's not necessary to have both Joel and myself as reviewers for this patch - since Joel's already started I'll leave this one to him. Feel free to have me as the reviewer for your next Talos bug though instead of Joel if you like.
Attachment #8862310 - Flags: review?(rwood)
(In reply to Robert Wood [:rwood] from comment #8)

> Hey Ionut, FYI as I mentioned on IRC, you can use 'hg commit --amend' and
> then push that to mozreview when submitting more changes to an existing
> patch that is under review - then it will update the existing commit in
> MozReview. :)

Thank you, Robert, for reminding me that. I read and followed the steps described in the Mozilla docs, and forgot about the "--amend" flag.
lets work on this in San Francisco.
Whiteboard: [PI:June]
Attachment #8862310 - Attachment is obsolete: true
Comment on attachment 8861884 [details]
Bug 1355934 - transform tpaint into a Pageloader test

https://reviewboard.mozilla.org/r/133900/#review158518

one nit- please push to try: |./mach try -b o -p linux64,macosx64,win32,win64 -u none -t other-e10s --rebuild 3|

::: testing/talos/talos/test.py
(Diff revision 2)
> -    timeout = 300
> -    gecko_profile_interval = 1
> -    gecko_profile_entries = 2000000
> -    tpmozafterpaint = True
> -    filters = filter.ignore_first.prepare(5) + filter.median.prepare()
> -    unit = 'ms'

you are missing unit='ms' in the new definition.
Attachment #8861884 - Flags: review?(jmaher) → review+
Comment on attachment 8861884 [details]
Bug 1355934 - transform tpaint into a Pageloader test

https://reviewboard.mozilla.org/r/133900/#review158726
all is well except the unit='ms'
Comment on attachment 8861884 [details]
Bug 1355934 - transform tpaint into a Pageloader test

https://reviewboard.mozilla.org/r/133900/#review158840
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 2896bcc63b55 -d 6acce622c977: rebasing 404867:2896bcc63b55 "Bug 1355934 - transform tpaint into a Pageloader test r=jmaher" (tip rename_tpaint)
local [dest] changed testing/talos/talos/startup_test/tpaint.html which other [source] deleted
use (c)hanged version, (d)elete, or leave (u)nresolved? u
merging testing/talos/talos/test.py
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment on attachment 8861884 [details]
Bug 1355934 - transform tpaint into a Pageloader test

https://reviewboard.mozilla.org/r/133900/#review158930
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67ea907faf0d
transform tpaint into a Pageloader test r=jmaher
Comment on attachment 8861884 [details]
Bug 1355934 - transform tpaint into a Pageloader test

https://reviewboard.mozilla.org/r/133900/#review159562

carry forward r+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 16db3836b7ca -d 8122909722c4: rebasing 405290:16db3836b7ca "Bug 1355934 - transform tpaint into a Pageloader test r=jmaher" (tip rename_tpaint)
merging testing/talos/talos/test.py
warning: conflicts while merging testing/talos/talos/test.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
:igoldan, it looks like you need to rebase your commit to the latest sources.
Ok, did that now and repushed to mozreview.
Flags: needinfo?(ionut.goldan)
I don't see a new push to mozreview, possibly something didn't work for you locally?
Whiteboard: [PI:June] → [PI:July]
That's odd, because locally the push to mozreview appeared to go ok. Anyway, I rebased once more, resolved some conflicts with wlach's changes on test.py, then repushed.
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/202262a3ddda
transform tpaint into a Pageloader test r=jmaher
https://hg.mozilla.org/mozilla-central/rev/202262a3ddda
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: