915156 - Port the performance testing framework to the new marionette runner

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[Mike Lee [:mlee]]

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.

Comment 2

[Hubert Figuiere [:hub]]

I'll take it. We need to go forward on that.

Julien if you object to it, let me know.

Comment 3

[Julien Wajsberg [:julienw]]

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 :)

Comment 4

[Hubert Figuiere [:hub]]

Porting the testing framework from xpcshell to Node.js is underway in the following branch

https://github.com/hfiguiere/gaia/tree/node-switch-over

Comment 5

[Julien Wajsberg [:julienw]]

I'm not sure this is the goal, but I may be wrong. Could be useful to have James peek this code :)

Comment 6

[Hubert Figuiere [:hub]]

(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"

Comment 7

[James Lal [:lightsofapollo]]

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.

Comment 8

[Hubert Figuiere [:hub]]

Attachment: Part 1.

Comment 9

[James Lal [:lightsofapollo]]

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.

Comment 10

[Hubert Figuiere [:hub]]

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

Comment 11

[Julien Wajsberg [:julienw]]

Will do tomorrow!

Comment 12

[Julien Wajsberg [:julienw]]

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 :)

Comment 13

[Hubert Figuiere [:hub]]

Attachment: Gaia fix for the perf event

Julien, I add to pick that piece of patch from bug 912541. Thanks.

Comment 14

[Hubert Figuiere [:hub]]

s/add/had/g

Comment 15

[Hubert Figuiere [:hub]]

Comment on attachment 833025 [details] [review]
Gaia fix for the perf event

picking the right reviewer. Thanks Vivien !

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 17

[Julien Wajsberg [:julienw]]

This is actually a good idea! :)

Comment 18

[Hubert Figuiere [:hub]]

Indeed event.target.src works.

Comment 20

[Hubert Figuiere [:hub]]

Comment on attachment 833025 [details] [review]
Gaia fix for the perf event

we have seen there is a better solution. obsolete.

Comment 21

[Hubert Figuiere [:hub]]

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.

Comment 22

[Julien Wajsberg [:julienw]]

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!

Comment 23

[Hubert Figuiere [:hub]]

Comment on attachment 829061 [details] [review]
Part 1.

3rd review?

Comment 24

[Julien Wajsberg [:julienw]]

Comment on attachment 829061 [details] [review]
Part 1.

Some more comments, and the event-based tests is not finishing correctly.

Comment 25

[Hubert Figuiere [:hub]]

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.

Comment 26

[Julien Wajsberg [:julienw]]

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.

Comment 27

[Hubert Figuiere [:hub]]

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.

Comment 28

[Julien Wajsberg [:julienw]]

(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.

Comment 29

[Julien Wajsberg [:julienw]]

Note: this is probably not a big thing to fix, I'm very confident that we'll be able to finish this this week.

Comment 30

[Hubert Figuiere [:hub]]

Oh I see, I never realised it was waiting like that.

Comment 31

[Hubert Figuiere [:hub]]

Indeed, marionetteScriptFinished() is called, but nothing happens.... until we timeout.

Comment 32

[Julien Wajsberg [:julienw]]

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

Comment 33

[Hubert Figuiere [:hub]]

Merged.

https://github.com/mozilla-b2g/gaia/commit/259cc53ad62489ab582142487f13e9df0984b808

Comment 34

[Hubert Figuiere [:hub]]

Follow up bug https://bugzilla.mozilla.org/show_bug.cgi?id=947259