[mozprocess] Remove ProcessHandler.waitForFinish()

RESOLVED FIXED in Firefox 63

Status

P3
normal
RESOLVED FIXED
8 months ago
5 months ago

People

(Reporter: whimboo, Assigned: achiwhane, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla63
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [lang=py])

User Story

To get started please check the following documentations:

https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html
https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

Attachments

(2 attachments, 3 obsolete attachments)

The method `ProcessHandler.waitForFinish()` doesn't seem to be used across the tree anymore. As such it can be removed. It can be found here:

https://dxr.mozilla.org/mozilla-central/rev/a466172aed4bc2afc21169b749b8068a4b98c93f/testing/mozbase/mozprocess/mozprocess/processhandler.py#866-870

When removed please run all the mozbase unit tests with `mach test testing/mozbase`. Once the patch is uploaded I will trigger a try build to test all other existing test jobs.
Priority: -- → P3

Comment 1

8 months ago
Hi,

I would like to work on this bug as my first open source contribution. I went through the two documentations listed and installed Mercurial on my system. I could not install MozReview as I don't have the access privileges. Could you please guide me further so that I can get started?
Thanks!
Flags: needinfo?(hskupin)
Hello Anuja, you are welcome to work on this bug. In regards of mozreview, if you refer to the more privileged features when having an LDAP account this is not necessary. The only feature you need is to upload patches and this should work fine once you setup the Bugzilla API key.
Flags: needinfo?(hskupin)

Comment 3

8 months ago
Hi Henrik,
Thank you! I am working on cloning mozilla-central and making the changes you suggested. Can I be assigned this bug?
As by our procedure I will assign you when the patch has been uploaded. Thanks for working on it.
(Assignee)

Comment 5

8 months ago
Created attachment 8984720 [details] [diff] [review]
Remove ProcessHandler.waitForFinish()

Hi,

As no one has uploaded a patch for the last 9 days, I've added a patch here. However, after I ran `mach test testing/mozbase` the `test_process_stop_via_multiple_threads[firefox]` test failed.
Attachment #8984720 - Flags: review?(hskupin)
(Assignee)

Comment 6

8 months ago
Created attachment 8984721 [details] [diff] [review]
Remove ProcessHandler.waitForFinish()

Hi,

As no one has uploaded a patch for the last 9 days, I've added a patch here. However, after I ran `mach test testing/mozbase` the `test_process_stop_via_multiple_threads[firefox]` test failed.
(Assignee)

Updated

8 months ago
Attachment #8984720 - Attachment is obsolete: true
Attachment #8984720 - Flags: review?(hskupin)
Akshay, that is fine. Thanks. Can you please tell me what exactly is failing? I don't see that this test is using this method we are about to remove via this bug.
(Assignee)

Comment 8

7 months ago
Created attachment 8984742 [details]
test_results

You're right in that the this test doesn't use the removed method, and it fails on line 35, `assert returncode not in [None, 0]`. I've attached my test results here.
Akshay, maybe this problem is only relate to your machine. I would kinda like to run the automated tests in CI with your patch attached. Would you mind to push the patch to mozreview instead of just uploading an attachment? That would help me a lot. Details about that can be found here:

https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#submitting-commits-for-review

Thanks
Flags: needinfo?(achiwhane)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

7 months ago
I've pushed the patch to mozreview.
Flags: needinfo?(achiwhane)
Thank you. I pushed a try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5679e5b48c352c173d85cd4e5e880deb0fb2b12
Assignee: nobody → achiwhane
Status: NEW → ASSIGNED
(Reporter)

Comment 13

7 months ago
mozreview-review
Comment on attachment 8985552 [details]
Bug 1465046 - Remove ProcessHandler.waitForFinish().

https://reviewboard.mozilla.org/r/251160/#review257830

So I checked the try build results. And as it looks like there is still a test which uses this method. You will have to update the test.

https://treeherder.mozilla.org/logviewer.html#?job_id=183499805&repo=try&lineNumber=47560-47577

It can be found here: https://dxr.mozilla.org/mozilla-central/source/config/tests/unit-nsinstall.py
Attachment #8985552 - Flags: review?(hskupin) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 15

7 months ago
I've updated the test to remove the deprecated function call so it calls wait() instead of waitForFinish().
Akshay, can you please use `hg histedit` to squash both commits into a single one? For this change there shouldn't be multiple commits. Thanks.
Flags: needinfo?(achiwhane)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8987373 - Attachment is obsolete: true
Attachment #8987373 - Flags: review?(hskupin)
(Assignee)

Comment 18

7 months ago
I've merged the commits and pushed the merged commit to mozreview.
Flags: needinfo?(achiwhane)
Comment on attachment 8984721 [details] [diff] [review]
Remove ProcessHandler.waitForFinish()

Usually a new push should update the existing mozreview patch, and not create a new one. But lets mark this old patch as obsolete now.
Attachment #8984721 - Attachment is obsolete: true
(Assignee)

Comment 20

7 months ago
Sorry! I'll keep that in mind for future patches.
(Reporter)

Comment 21

7 months ago
mozreview-review
Comment on attachment 8985552 [details]
Bug 1465046 - Remove ProcessHandler.waitForFinish().

https://reviewboard.mozilla.org/r/251160/#review260268

r=me with the commit message fixed. See inline comments for details.

::: commit-message-874de:7
(Diff revision 2)
> +
> +MozReview-Commit-ID: JQnHZrLLDLv
> +***
> +Bug 1465046 - Remove depricated function call in unit-nsinstall tests. r=whimboo
> +
> +MozReview-Commit-ID: DFAMmN8CFiH

Ok, here is the reason why a new mozreview has been created. You left in the commit message from the other commit, which then most likely created a new one for the first commit. Please remove everything after (including) the stars.

Also for the future I would propose that you don't add `r=whimboo` in the patch directly, but set it in mozreview directly.
Attachment #8985552 - Flags: review?(hskupin) → review+

Comment 22

6 months ago
is this bug still open ?
Akshay, as it looks like you missed to just update the commit message, and to push the patch again. Can you please do it so we can get your patch landed? If you aren't able to please let us know. Thanks.
Flags: needinfo?(achiwhane)
Given that we haven't heard from Akshay, I will just do the update of the commit message myself and land the patch.
Flags: needinfo?(achiwhane)

Comment 25

5 months ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a51308cf76a
Remove ProcessHandler.waitForFinish(). r=whimboo

Comment 26

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a51308cf76a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Thank you again Akshay for fixing this bug!
You need to log in before you can comment on or make changes to this bug.