stopping conditions should be improved for mozmill 1.5.2

RESOLVED FIXED

Status

Testing Graveyard
Mozmill
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: Jeff Hammel, Assigned: Jeff Hammel)

Tracking

Details

(Whiteboard: [mozmill-1.5.2+])

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
The general stopping framework has been improved as a side-effect of
the restructuring have to do with manifests:

https://github.com/mozautomation/mozmill/commit/5a58949f9628d7ebbcbe64ede2674c2100a8ea0a

This should be backported to 1.5.2 to improve its robustness as well.
(Assignee)

Updated

8 years ago
Assignee: nobody → jhammel
Whiteboard: [mozmill-1.5.2?]
Blocks: 615662
(Assignee)

Comment 1

8 years ago
Created attachment 497969 [details] [diff] [review]
patch to improve things

I'm not convinced this is 100%, even ignoring the SIGINT vs SIGKILL in killableprocess.  Its a good port from master, but the control flow has changed a lot
Attachment #497969 - Flags: review?(fayearthur+bugs)
The patch doesn't help for bug 615662, but fixes the problem reported on bug 619110.
Blocks: 619110
No longer blocks: 615662
(Assignee)

Comment 3

8 years ago
I'm not sure where our logic fails in 1.5.2 for stopping.  As a hacky fix, I would suggest putting in a SIGKILL if the process hasn't shut down (which *may*? fix bug 615662).  The control flow is much simpler on master: https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/__init__.py#L347 I'm not sure if this is an issue there.  If it is, we should make mozrunner.cleanup more robust.
Comment on attachment 497969 [details] [diff] [review]
patch to improve things

Definitely looks like an improvement, thanks.
Attachment #497969 - Flags: review?(fayearthur+bugs) → review+
(Assignee)

Comment 5

8 years ago
:ctalbert, :whimboo: okay to mozmill-1.5.2+ so I can land?

Comment 6

8 years ago
(In reply to comment #5)
> :ctalbert, :whimboo: okay to mozmill-1.5.2+ so I can land?

yes, of course.
Whiteboard: [mozmill-1.5.2?] → [mozmill-1.5.2+]
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.