Closed Bug 1064280 Opened 5 years ago Closed 5 years ago

Add automated test for changing the URL in a pinned tab

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: mihaelav, Assigned: mihaelav)

References

Details

Attachments

(1 file, 2 obsolete files)

Add automated test for bug 1039904:

Steps:
1. Start Firefox
2. Pin a tab
3. Change the URL in the pinned tab

Expected result: URL loads in the same (pinned) tab; no additional tabs are created
Assignee: nobody → mihaela.velimiroviciu
Attached patch test v1 (obsolete) — Splinter Review
Dão, can you please review the test, since you also fixed the bug?

Thank you!
Attachment #8490623 - Flags: review?(dao)
(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #1)
> Created attachment 8490623 [details] [diff] [review]
> test v1
> 
> Dão, can you please review the test, since you also fixed the bug?
> 
> Thank you!

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=07da0d8b2b85
Comment on attachment 8490623 [details] [diff] [review]
test v1

>+add_task(function(){
>+  info("Test that changing the URL in a pinned tab works correctly");

Please make this a comment instead:

// Test that changing the URL in a pinned tab works correctly

>+add_task(function asyncCleanup() {
>+  gBrowser.removeTab(gBrowser.selectedTab);

Can you use registerCleanupFunction?

>+  info("App tab was closed");

Please remove this.

>+function waitForPageLoad() {
>+  let deferred = Promise.defer();
>+
>+  function onTabLoad(event) {
>+    gBrowser.removeEventListener("load", onTabLoad, true);
>+    info("Load tab event received");
>+    deferred.resolve();
>+  }
>+
>+  gBrowser.addEventListener("load", onTabLoad, true, true);
>+  return deferred.promise;
>+}

Can you just use promiseTabLoadEvent?
Attachment #8490623 - Flags: review?(dao)
Attachment #8490623 - Flags: review-
Attachment #8490623 - Flags: feedback+
Attached patch v1.1 (obsolete) — Splinter Review
updated based on review
Attachment #8490623 - Attachment is obsolete: true
Attachment #8493036 - Flags: review?(dao)
Comment on attachment 8493036 [details] [diff] [review]
v1.1

>+  let promisePageload = promiseTabLoadEvent(appTab); //waitForPageLoad();

please remove that comment

>+  is(appTab.linkedBrowser.contentDocument.location.href, TEST_LINK_CHANGED,
>+     "New page loaded in the app tab");

please use appTab.linkedBrowser.currentURI.spec instead of appTab.linkedBrowser.contentDocument.location.href

>+registerCleanupFunction(function () {
>+    gBrowser.removeTab(gBrowser.selectedTab);
>+  });

indentation is off, should be like this:

registerCleanupFunction(function () {
  gBrowser.removeTab(gBrowser.selectedTab);
});
Attachment #8493036 - Flags: review?(dao) → review+
Attached patch v1.2Splinter Review
Thanks Dão for the review! Fixed the nits. Carrying over the r+
Attachment #8493036 - Attachment is obsolete: true
Attachment #8494319 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/daddf970831d
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/daddf970831d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.