Closed Bug 1465046 Opened 6 years ago Closed 6 years ago

[mozprocess] Remove ProcessHandler.waitForFinish()

Categories

(Testing :: Mozbase, enhancement, P3)

Product:
Testing
Component:
Mozbase
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: whimboo, Assigned: achiwhane, Mentored)

Details

(Keywords: good-first-bug, 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 files, 3 obsolete files)

test_results
86.51 KB, text/plain
Details
Bug 1465046 - Remove ProcessHandler.waitForFinish().
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
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
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)
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.
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)
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 - 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.
Attached file 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)
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
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-
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)
Attachment #8987373 - Attachment is obsolete: true
Attachment #8987373 - Flags: review?(hskupin)
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
Sorry! I'll keep that in mind for future patches.
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+
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)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a51308cf76a
Remove ProcessHandler.waitForFinish(). r=whimboo
https://hg.mozilla.org/mozilla-central/rev/3a51308cf76a
Status: ASSIGNED → RESOLVED
Closed: 6 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: