Closed Bug 1398069 Opened 2 years ago Closed 2 years ago

Ensure talos account for 'cycles' and coalesce results

Categories

(Testing :: Talos, enhancement)

enhancement
Not set

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Whiteboard: [PI:September])

Attachments

(1 file)

See bug 1394804 comment 30:
> I see that we run this 5 times, but oddly it seems as though we have a reporting > problem- the resulting PERFHERDER_JSON had 5 duplicate copies of the data (25 
> replicates/subtest - 5 identical copies) where as normally we just publish a
> single copy of the data.

> Please file a bug to work on that, specifically ensure that we account for 
> 'cycles' and coalesce results, here is the code area:
> http://searchfox.org/mozilla-central/source/testing/talos/talos/output.py#58
This code is very hard to follow with all these result/results/tresults/tpresults,
but this patch got things to work locally for my DAMP needs.

I have *absolutely no idea* if that break any other test involving this codepath.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eefd4df86b3cd71731d06cc1682c1f18f392f70
Should I ask someone else to review this patch?

It prevents me from proceeding with bug 1394804.
Flags: needinfo?(jmaher)
Comment on attachment 8906695 [details]
Bug 1398069 - Merge results when using cycles > 1.

I had looked at this and was not sure if it was right/wrong or what could be missing.  :rwood, can you pick this up tomorrow and figure it out.  This is not an area of the talos code that we are both very familiar with
Flags: needinfo?(jmaher)
Attachment #8906695 - Flags: review?(jmaher) → review?(rwood)
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #4)
...
Ok I'll have a look also
FYI, I added the rest of the talos jobs to the try run in comment 2
Update: Spent some time looking into tppagecycles vs tpcycles vs cycles and I now understand the issue. For example, if you run the existing DAMP with --tppagecycles 2 and -cycles 1, you get 2 replicates per test case/subtest as expected. If you run --tppagecycles 2 and -cycles 2 the entire suite is repeated as expected however for the talos.json output I had expected to see the first set of results/subtests each with 2 replicates, and then all the same subtests listed again with 2 more replicates. However what you see is the first set of subtests output with each subtest listing all 4 replicates, and then all of the subtests output repeated again and listing the exact same 4 replicate values over again.
Update: I applied the patch locally and ran damp, and confirmed with --tppagecycles 2 and -cycles 2, the output now just contains one set of all the subtests, each subtest having 4 replicates listed, as expected.

Note: ts_paint, sessionrestore, and tresize tests are the other tests that use cycles = N where N is not the default of 1; so I am checking to see how this patch will effect the output of those tests.
Comment on attachment 8906695 [details]
Bug 1398069 - Merge results when using cycles > 1.

Yep looks good! Also verified this patch has no effect on the other talos tests that use cycles > 1 by default; they still report the same way.
Attachment #8906695 - Flags: review?(rwood) → review+
r+ wasn't forwarded to mozreview, so requesting checkin.
I'm not sure this keyword is still watched??
Keywords: checkin-needed
Attachment #8906695 - Flags: review+ → review?(rwood)
Attachment #8906695 - Flags: review?(rwood) → review+
Assignee: nobody → poirot.alex
This doesn't meet the review requirements in MozReview for autoland to push it.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits
Keywords: checkin-needed
:rwood and :ochameau are both consistent contributors to the repo- are they not l3?  who is not l3?
Will it not land in mozreview maybe because the commit text is 'r=jmaher' however I am the one who reviewed it? Mozreview won't allow me to 'ship it'.
Thanks Alexandre, even with r=rwood, in mozreview no 'ship it' option appears for me even when I'm logged in. Not sure what is going on...
:jmaher, are you able to mark this review as 'ship it'?

https://reviewboard.mozilla.org/r/178428/
Flags: needinfo?(jmaher)
it appears so
Flags: needinfo?(jmaher)
Not a good day to land patches:
  "A server error occurred: Autoland returned an error message during communications."
I assume this is because autoland is closed right now
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31376e1efa8f
Merge results when using cycles > 1. r=jmaher
https://hg.mozilla.org/mozilla-central/rev/31376e1efa8f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.