Closed Bug 915156 Opened 6 years ago Closed 6 years ago

Port the performance testing framework to the new marionette runner

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.2 C5(Nov22)

People

(Reporter: julienw, Assigned: hub)

References

Details

(Keywords: perf, Whiteboard: [c=automation p=4 s= u=])

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
jlal
: review+
julienw
: review+
Details | Review
No description provided.
Blocks: 915158
No longer blocks: 915158
Status: NEW → ASSIGNED
Keywords: perf
Whiteboard: [c=instrumentation p= s= u=]
Blocks: 916017
Blocks: 916031
Whiteboard: [c=instrumentation p= s= u=] → [c=automation p= s= u=]
Blocks: 819080
Julien, any update on this? Are you working on this and if not, do you know when you might? It's blocking several important performance automation efforts.
Flags: needinfo?(felash)
I'll take it. We need to go forward on that.

Julien if you object to it, let me know.
Assignee: felash → hub
Flags: needinfo?(felash)
Whiteboard: [c=automation p= s= u=] → [c=automation p=4 s= u=]
Please do!

I really wanted to do it quickly but the truth is that we all have more important things in our respective teams. I'd be very happy if the performance team would do this :)
Blocks: 915158
Blocks: 888099
Porting the testing framework from xpcshell to Node.js is underway in the following branch

https://github.com/hfiguiere/gaia/tree/node-switch-over
No longer blocks: 888099
I'm not sure this is the goal, but I may be wrong. Could be useful to have James peek this code :)
Blocks: 844032
(In reply to Julien Wajsberg [:julienw] from comment #5)
> I'm not sure this is the goal, but I may be wrong. Could be useful to have
> James peek this code :)

Yes it is. According to James, "anything that was designed to work with xpcshell needs to die fast"
The new integration tests _ONLY_ work on node... There never again will be an xpchshell marionette-js-client. I hope I was very clear about this during our original chats.
Attached file Part 1.
Attachment #829061 - Flags: review?(jlal)
Comment on attachment 829061 [details] [review]
Part 1.

Giving a very high level r+ (mostly on the deletes that where my mode :))

I wrote the original marionette client and a bunch of horrible xpcshell extensions that made it glue. I am comfortable reviewing that stuff (deletes :D) and extensions needed for the new runner.

Most of the other code :Rik, :etienne_s and :julienw wrote and I don't feel super comfortable as the final word there (the performance framework bits).

I am happy to support you in whatever way makes sense but I think we need another reviewer for the performance framework bits who can also act as a future maintainer.
Attachment #829061 - Flags: review?(jlal) → review+
Comment on attachment 829061 [details] [review]
Part 1.

Julien, can we get a review? I have more fixes pending directly for the test to actually produce the results I'd like to push in a later patch.


Thanks
Attachment #829061 - Flags: review?(felash)
Will do tomorrow!
No longer blocks: 915158
Comment on attachment 829061 [details] [review]
Part 1.

I can't really r+ on an unfinished patch.

You can squash your current commits, fix the whitespaces in a separate commit, and then continue your work on another commit, so that the new changes will be easy to review.

Please ask review again once you're ready :)
Attachment #829061 - Flags: review?(felash)
Target Milestone: --- → 1.2 C5(Nov22)
Attached file Gaia fix for the perf event (obsolete) —
Julien, I add to pick that piece of patch from bug 912541. Thanks.
Attachment #833025 - Flags: review?(felash)
s/add/had/g
Comment on attachment 833025 [details] [review]
Gaia fix for the perf event

picking the right reviewer. Thanks Vivien !
Attachment #833025 - Flags: review?(felash) → review?(21)
Comment on attachment 833025 [details] [review]
Gaia fix for the perf event

Can you not simply get the src attribute of the evt.target since the event is dispatched on the iframe itself?

Please re-ask r? if there is any technical reasons why you can't.
Attachment #833025 - Flags: review?(21)
This is actually a good idea! :)
Indeed event.target.src works.
Duplicate of this bug: 888099
Comment on attachment 833025 [details] [review]
Gaia fix for the perf event

we have seen there is a better solution. obsolete.
Attachment #833025 - Attachment is obsolete: true
Comment on attachment 829061 [details] [review]
Part 1.

Julien, this is the up to date branch. All the known test run on my inari and the performance data is output correctly.

There are a few commits to address previous comments, and then a big one for part 2.
Attachment #829061 - Flags: review?(felash)
Comment on attachment 829061 [details] [review]
Part 1.

review finished, please ask me a review again once you're ready.
(I put "r-" so that we see there is not only James' r+, but don't take offense ;) )

For next review, please have 3 commits: 1 for the first review, 1 for this review, and 1 for the next review.

Thanks!
Attachment #829061 - Flags: review?(felash) → review-
Blocks: 942201
Blocks: 942203
Comment on attachment 829061 [details] [review]
Part 1.

3rd review?
Attachment #829061 - Flags: review- → review?(felash)
Comment on attachment 829061 [details] [review]
Part 1.

Some more comments, and the event-based tests is not finishing correctly.
Attachment #829061 - Flags: review?(felash)
Attachment #829061 - Flags: review-
Depends on: 944494
Comment on attachment 829061 [details] [review]
Part 1.

Event based test works here.

Also fixed most of the issue.

Maybe we can merge this and follow up. It is still way better than last time.
Attachment #829061 - Flags: review- → review?(felash)
Blocks: 945387
Comment on attachment 829061 [details] [review]
Part 1.

I agree this is way better than the previous state.

I still want the startup event test to work properly before merging this though.
Attachment #829061 - Flags: review?(felash)
As I said on Github, I had all the startup event test work for contacts (which is the only app that has one) with a medium reference workload, as well as fm (see work on bug 819080)

I pushed the nit fixes to the PR.
Attachment #829061 - Flags: review?(felash)
(In reply to Hubert Figuiere [:hub] from comment #27)
> As I said on Github, I had all the startup event test work for contacts
> (which is the only app that has one) with a medium reference workload, as
> well as fm (see work on bug 819080)
> 
> I pushed the nit fixes to the PR.

see https://github.com/mozilla-b2g/gaia/pull/13497#issuecomment-29890013 for more explanation. I was probably not clear enough in my previous comment.

The output is correct, which does not mean that it works as expected. We need the test to stop in a timely manner, instead of waiting for mocha's timeout.
Attachment #829061 - Flags: review?(felash)
Note: this is probably not a big thing to fix, I'm very confident that we'll be able to finish this this week.
Oh I see, I never realised it was waiting like that.
Indeed, marionetteScriptFinished() is called, but nothing happens.... until we timeout.
Comment on attachment 829061 [details] [review]
Part 1.

Since this is deeper than expected, r=me, but please file a follow-up bug to investigate whether it's:
* an issue in marionette 
* an issue in the marionette js client
* an issue in our use of the marionette js client

and fix it
Attachment #829061 - Flags: review+
Blocks: 947259
Follow up bug https://bugzilla.mozilla.org/show_bug.cgi?id=947259
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.