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

RESOLVED FIXED in Firefox 56

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwatt, Assigned: igoldan)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [PI:July])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
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)
No longer depends on: 1357512
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
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 hidden (mozreview-request)

Comment 5

2 years ago
mozreview-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]
Comment hidden (mozreview-request)
Attachment #8862310 - Attachment is obsolete: true

Comment 13

2 years ago
mozreview-review
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 14

2 years ago
mozreview-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 hidden (mozreview-request)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8861884 [details]
Bug 1355934 - transform tpaint into a Pageloader test

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

Comment 18

2 years ago
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 hidden (mozreview-request)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8861884 [details]
Bug 1355934 - transform tpaint into a Pageloader test

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

Comment 21

2 years ago
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67ea907faf0d
transform tpaint into a Pageloader test r=jmaher
Comment hidden (mozreview-request)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8861884 [details]
Bug 1355934 - transform tpaint into a Pageloader test

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

carry forward r+

Comment 25

2 years ago
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]
Comment hidden (mozreview-request)
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.

Comment 31

2 years ago
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/202262a3ddda
transform tpaint into a Pageloader test r=jmaher

Comment 32

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/202262a3ddda
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.