Closed Bug 960063 Opened 10 years ago Closed 10 years ago

Superfluous Marionette session breaks update-dashboard.py

Categories

(Testing Graveyard :: Eideticker, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: davehunt, Assigned: davehunt)

References

Details

Attachments

(1 file, 2 obsolete files)

Since bug 942826 we create a Marionette instance and session whenever we instantiate device.B2GADB, even if it's not needed. This causes an issue if you instantiate this multiple times without the previous Marionette session being deleted.
It might be better to avoid creating this Marionette instance/session, or reusing it instead of creating a new one. This patch simply deletes it if it's been created.
Attachment #8360422 - Flags: review?(wlachance)
Blocks: 904837
Blocks: 916989
Comment on attachment 8360422 [details] [diff] [review]
Delete superfluous Marionette session in update-dashboard.py. v1.0

Yeah, I think patching things here after-the-fact seems kind of wrong.

As you suggest, I think I would prefer a class argument to B2GADB that makes creating the marionette session optional, I'll attach a patch that I think would work here.
Attachment #8360422 - Flags: review?(wlachance) → review-
Summary: Delete superfluous Marionette session in update-dashboard.py → Superfluous Marionette session breaks update-dashboard.py
Attachment #8360808 - Flags: review?(dave.hunt)
Comment on attachment 8360808 [details] [diff] [review]
Make it possible not to create a marionette session for device;r=davehunt

I'm running into more problems apparently caused by this... I am wondering now if setting up a marionette session automatically on creation of a B2GADB object is the right behaviour. Maybe we should force the user of this API to do that explicitly. Will investigate further.
Attachment #8360808 - Flags: review?(dave.hunt)
David Burns suggested that this might be related to bug 959730.
(In reply to Dave Hunt (:davehunt) from comment #5)
> David Burns suggested that this might be related to bug 959730.

I don't think that's the problem... the __del__ method cleans up emulator and gecko instances, of which we have none here.
Ok, finally figured out what the problem was. After restarting b2g, we create a new marionette session, which never gets cleaned up. I filed bug 960931 about making it more obvious that this was an error. I'll try to fix this up asap. Meanwhile, in eideticker I think the solution is to not actually connect to marionette in the B2GADB class. That should be an optional additional setup step.
(In reply to William Lachance (:wlach) from comment #6)
> (In reply to Dave Hunt (:davehunt) from comment #5)
> > David Burns suggested that this might be related to bug 959730.
> 
> I don't think that's the problem... the __del__ method cleans up emulator
> and gecko instances, of which we have none here.

Shouldn't it also delete any active sessions?
That sounds reasonable, the only minor issue I can think of is the case where the Marionette server becomes unresponsive, so cleanup() will end up taking as long as the timeout period for socket.timeout.
It was easy to inadvertently do this before, as the device class we used for
B2G created a marionette session by default. Let's not do that. Also cleaned
up a bunch of logic in the test runner to make it easier to follow.
Comment on attachment 8362576 [details] [diff] [review]
Make sure we don't try to have two concurrent marionette sessions;r=davehunt

This essentially does the same thing as your earlier patch, but it cleans up a bunch of logic and adds some comments to hopefully make it clearer what's going on.
Attachment #8362576 - Flags: review?(dave.hunt)
Attachment #8360808 - Attachment is obsolete: true
Attachment #8360422 - Attachment is obsolete: true
Comment on attachment 8362576 [details] [diff] [review]
Make sure we don't try to have two concurrent marionette sessions;r=davehunt

Review of attachment 8362576 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but I'm unable to apply it. How are you generating your patches?
Attachment #8362576 - Flags: review?(dave.hunt) → review+
I was able to apply this by falling back to a 3-way merge:

$ git apply --3way bug960063.patch

Will now test and if the results are good I'll land it.
Landed in:
https://github.com/mozilla/eideticker/commit/ffaf504ec0abf64b93ebcc31922bf88ecb4910ca
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: