Replace promiseTopicObserved with TestUtils.topicObserved in browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: johannh, Assigned: ryanro1997, Mentored)

Tracking

({good-first-bug})

60 Branch
Firefox 61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

a year ago
This is a good first bug for newcomers to Firefox development.

promiseTopicObserved in the browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js test file can be replaced by the TestUtils.topicObserved[0] utility function.

We need to replace this occurrence: https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/browser/base/content/test/general/browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js#107

It seems to me that this is also the last time we use the promiseTopicObserved function in browser/base/content/test/general, which means we can also remove its definition here: https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/browser/base/content/test/general/head.js#540


For instructions on how to get your local build of Firefox up and running and submit your patch, see https://developer.mozilla.org/en-US/docs/Introduction.

You can run this test with the ./mach mochitest command:

./mach mochitest browser/base/content/test/general/browser_testOpenNewRemoteTabsFromNonRemoteBrowsers.js

Please leave a comment if you would like to be assigned to this bug and feel free to ask questions here or via IRC if you're stuck.

[0] https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/testing/modules/TestUtils.jsm#56

Comment 1

a year ago
I would like to work on this issue. Is it available?

Comment 2

a year ago
Can I work on the bug?
Reporter

Comment 3

a year ago
utsha.sinha1510 was first :)

(In reply to kev.ftw595 from comment #2)
> Can I work on the bug?

How about bug 1447941 instead? :)
Assignee: nobody → utsha.sinha1510
Status: NEW → ASSIGNED
Reporter

Comment 4

a year ago
Hi, are you still working on this? Is there anything I can help you with?
Flags: needinfo?(utsha.sinha1510)

Comment 5

a year ago
Go ahead. Take over. I'm kind of new and it will take time to get acquainted.
Flags: needinfo?(utsha.sinha1510)
Reporter

Comment 6

a year ago
Thanks!
Assignee: utsha.sinha1510 → nobody
Status: ASSIGNED → NEW
Assignee

Comment 7

a year ago
Hello, I would like to be assigned this bug.
Reporter

Comment 8

a year ago
Of course, let me know if you need any help.

Thank you!
Assignee: nobody → ryanro1997
Status: NEW → ASSIGNED
Assignee

Comment 9

a year ago
I was wondering if there was a way to fix:
FAIL test left unexpected property on window: TestUtils -
which happens after this info message:
Leaving test bound test_new_window
All other tests pass.

Thanks.
Flags: needinfo?(jhofmann)
Assignee

Comment 10

a year ago
(In reply to ryanro1997 from comment #9)
> I was wondering if there was a way to fix:
> FAIL test left unexpected property on window: TestUtils -
> which happens after this info message:
> Leaving test bound test_new_window
> All other tests pass.
> 
> Thanks.

It seems it was due to me adding the line:
ChromeUtils.import("resource://testing-common/TestUtils.jsm");
Which was not needed.
Flags: needinfo?(jhofmann)
Assignee

Updated

a year ago
Attachment #8965286 - Flags: review?(jhofmann)
Assignee

Updated

a year ago
Attachment #8965286 - Attachment description: Bug 1445267 - Removed deprecated definition promiseTopicObserved and replaced call with TestUtils.topicObserved.patch → Bug 1445267 - Removed deprecated definition promiseTopicObserved and replaced call with TestUtils.topicObserved, r=jhofmann.patch
Attachment #8965286 - Attachment filename: oved deprecated definition promiseTopicObserved and replaced call with TestUtils.topicObserved.patch → ted definition promiseTopicObserved and replaced call with TestUtils.topicObserved, r=jhofmann.patch
Assignee

Updated

a year ago
Attachment #8965286 - Attachment filename: ted definition promiseTopicObserved and replaced call with TestUtils.topicObserved, r=jhofmann.patch → Bug 1445267.patch
Reporter

Comment 12

a year ago
Comment on attachment 8965286 [details] [diff] [review]
Bug 1445267 - Removed deprecated definition promiseTopicObserved and replaced call with TestUtils.topicObserved, r=jhofmann.patch

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

Ah, excellent! This looks good. Let's give it a quick run on our Try server (https://wiki.mozilla.org/ReleaseEngineering/TryServer), to verify that all tests are still passing. I'll also set the checkin-needed flag so that someone will land the patch when try is green.

Thanks!
Attachment #8965286 - Flags: review?(jhofmann) → review+
Reporter

Updated

a year ago
Keywords: checkin-needed
Reporter

Comment 14

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/af78c390b5d2b9d114093836b9c811249211ceda
Bug 1445267 - Removed deprecated definition promiseTopicObserved and replaced call with TestUtils.topicObserved. r=johannh
Reporter

Updated

a year ago
Keywords: checkin-needed

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/af78c390b5d2
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.