Closed Bug 1398069 Opened 2 years ago Closed 2 years ago

Ensure talos account for 'cycles' and coalesce results


(Testing :: Talos, enhancement)

Not set


(firefox57 fixed)

Tracking Status
firefox57 --- fixed


(Reporter: ochameau, Assigned: ochameau)



(Whiteboard: [PI:September])


(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:
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.
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.
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'?
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
Merge results when using cycles > 1. r=jmaher
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.