[marionette-js-runner] Call a "shutdown" or "suiteStop" function in the host instead of testStop

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Right now marionette-js-runner calls the host's stop method on suiteTeardown:
https://github.com/mozilla-b2g/marionette-js-runner/blob/master/lib/runtime/hostmanager.js#L111

This is the same method that is used to restart the host between tests. But on suiteTeardown, the host might need to perform additional cleanup (e.g close a socket in this case).

This patch worked locally:
https://github.com/ahal/marionette-js-runner/commit/e2b1d794c850c1deb6def5547504faa0a283fa11

This requires a change to whatever hosts are being used as well.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Attachment #8404924 - Flags: review?(gaye)
Gareth, I'm close to the point where this is going to blocking me, is this change ok?
Flags: needinfo?(gaye)
Sorry if this is obvious, but why is it that you can't build this into something that implements the host api? Host#stop is intentionally async so that you can do things like this.
Flags: needinfo?(gaye)
(In reply to Gareth Aye [:gaye] from comment #3)
> Sorry if this is obvious, but why is it that you can't build this into
> something that implements the host api? Host#stop is intentionally async so
> that you can do things like this.

I'm not sure, maybe that's possible. How can the host tell whether it's the last test or not though?
(In reply to Andrew Halberstadt [:ahal] from comment #4)
> (In reply to Gareth Aye [:gaye] from comment #3)
> > Sorry if this is obvious, but why is it that you can't build this into
> > something that implements the host api? Host#stop is intentionally async so
> > that you can do things like this.
> 
> I'm not sure, maybe that's possible. How can the host tell whether it's the
> last test or not though?

The host doesn't need to know. So long as you implement #start and #stop, things will work out. As things are right now,

https://github.com/mozilla-b2g/marionette-js-runner/blob/master/lib/runtime/hostmanager.js#L54 should call #start on your host at the beginning of the test suite,

https://github.com/mozilla-b2g/marionette-js-runner/blob/master/lib/runtime/hostmanager.js#L105 calls #stop and then #start after each test case completes, and then

https://github.com/mozilla-b2g/marionette-js-runner/blob/master/lib/runtime/hostmanager.js#L112 should call #stop one last time at the end.

Does that make sense / solve the problem?
It doesn't need to know currently.. but it might do things like open sockets that it should close after the last test. We want to clean up at the end, but we don't want to open/close the socket each time. So it's no longer enough to stop the host at the end and hope everything is ok.
Comment on attachment 8404924 [details] [review]
Give host a chance to cleanup on suiteTeardown

I'm removing the review for now because I think that pull has bitrotted anyway. I still think this is something that should be fixed though. I can get around it by creating/destroying a new socket between each test, but if we had a suiteTeardown function that gets called, there wouldn't be any need to do that.
Attachment #8404924 - Flags: review?(gaye)
I thought of another reason why this is needed. I'm using the 'structured log' protocol that we're in the process of standardizing all of our test harnesses on:
http://mozbase.readthedocs.org/en/latest/mozlog_structured.html#data-format

It requires that I send a suite_start and a suite_end message. It is currently not possible to send a suite_end message because on Host.stop() the host has no idea whether that was the last test in the suite or not. If there was a Host.suiteEnd event, then this would be possible while also allowing me to cleanup the socket properly (without a gross hack).

Does this make sense Gareth? Out of curiosity, are you opposed to this change? If so, how come?
Flags: needinfo?(gaye)
This is now blocking several things related to gaia-integration on devices. Without this patch, it isn't possible to log suite_start and suite_end messages.
Attachment #8404924 - Attachment is obsolete: true
Attachment #8461085 - Flags: review?(gaye)
Flags: needinfo?(gaye)
Blocks: 1043385
Comment on attachment 8461085 [details] [review]
Call host.teardown() (if it exists) on suiteTeardown

I haven't been able to reach Gareth (here or on irc) for quite some time now, I guess he's on pto?

James, would you mind taking a look at this?
Attachment #8461085 - Flags: review?(gaye) → review?(jlal)
Comment on attachment 8461085 [details] [review]
Call host.teardown() (if it exists) on suiteTeardown

James went ahead and merged it, so I guess that means it was r+'ed? :p

https://github.com/mozilla-b2g/marionette-js-runner/commit/27883e56343ad6be4eb00b843fa3a1110038593e
Attachment #8461085 - Flags: review?(jlal) → review+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.