1113178 - Replace removeAllPages with history.clear() in tests

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

once we have the new clear() API we should replace all removeAllPages calls with clear() calls in tests (some tests out of Places folders are also using removeAllPages and should be converted)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1113178-Replace-removeAllPages-with-history.clea.patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=53b5657f83f2

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1113178-Replace-removeAllPages-with-history.clea.patch, v2

Fixed toolkit/components/places/tests/unit/test_frecency_observers.js timeouts.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8ab70ac4911

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1113178-Replace-removeAllPages-with-history.clea.patch, v3

Found out that PlacesUtils.history proxies History.jsm methods. Using that now.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8548841 [details] [diff] [review]
0002-Bug-1113178-Replace-removeAllPages-with-history.clea.patch, v3

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

We are not ready to drop expiration topic waiting, for now.
We need a separate bug for that, it would be safer.

::: browser/base/content/test/general/browser_bug581253.js
@@ +74,5 @@
>   * Clears history invoking callback when done.
>   */
>  function waitForClearHistory(aCallback)
>  {
> +  PlacesUtils.history.clear().then(aCallback);

unfortunately we must still wait for TOPIC_EXPIRATION_FINISHED cause clear history happens half in clear and half in expiration.
I'd suggest to use promiseClearHistory() and throw away this helper.

::: browser/base/content/test/general/browser_sanitizeDialog.js
@@ +975,5 @@
>  /**
>   * Removes all history visits, downloads, and form entries.
>   */
>  function blankSlate() {
> +  let clearHistory = PlacesUtils.history.clear();

As well as here, I'd suggest using promiseClearHistory() directly in Promise.all

@@ +980,2 @@
>  
> +  let deleteDownloads = Task.spawn(function deleteAllDownloads() {

function*

@@ +994,5 @@
> +          resolve();
> +        }
> +      },
> +
> +      handleError(error) {

nit: I'd remove that empty newline, it confuses the readability code flow, imo

@@ +995,5 @@
> +        }
> +      },
> +
> +      handleError(error) {
> +        do_throw("Error occurred updating form history: " + error);

I don't think do_throw works in a mochitest-browser test?
I see other few browser tests are using it... likely by a copy/paste mistake in the past

::: browser/base/content/test/general/head.js
@@ +388,1 @@
>  }

you should not change promiseClearHistory, we still need to wait for the expiration topic.

::: browser/base/content/test/newtab/head.js
@@ +240,5 @@
>    });
>  }
>  
>  function clearHistory(aCallback) {
> +  PlacesUtils.history.clear().then(aCallback);

please replace this helper with promiseClearHistory

I'd suggest to copy promiseClearHistory to PlacesTestUtils, import PlacesTestUtils in head.js and use that.

::: browser/components/places/tests/browser/browser_bookmarksProperties.js
@@ +478,1 @@
>    }

nit: cleanup() promiseClearHistory();

::: browser/components/places/tests/browser/head.js
@@ +80,5 @@
>   * @param aCallback
>   *        Function to be called when done.
>   */
>  function waitForClearHistory(aCallback) {
> +  promiseClearHistory().then(aCallback);

Please file a (mentored?) follow-up to get rid of all waitForClearHistory helpers and use the promiseClearHistory() version everywhere

@@ +376,5 @@
>   * @resolves When history has been cleared.
>   * @rejects Never.
>   */
>  function promiseClearHistory() {
> +  return PlacesUtils.history.clear();

We can't change this, for the same reason as above.

As I said on IRC, we need a separate bug to handle re-merging expiration in clear so that just waiting for the clear promise will be enough.

::: browser/components/places/tests/chrome/head.js
@@ +57,5 @@
>  /**
>   * Clears history invoking callback when done.
>   */
>  function waitForClearHistory(aCallback) {
> +  PlacesUtils.history.clear().then(aCallback);

promiseClearHistory

::: browser/components/preferences/tests/browser_chunk_permissions.js
@@ +63,5 @@
>  }
>  
>  function runNextTest() {
>    if (gTestIndex == tests.length) {
> +    PlacesUtils.history.clear().then(finish);

promiseClearHistory (once added to PlacesTestUtils)

::: browser/components/preferences/tests/browser_permissions.js
@@ +80,5 @@
>  }
>  
>  function runNextTest() {
>    if (gTestIndex == tests.length) {
> +    PlacesUtils.history.clear().then(finish);

ditto

@@ +274,5 @@
>  
>    function test_forget_site() {
>      // click "Forget About This Site" button
>      gBrowser.contentDocument.getElementById("forget-site-button").doCommand();
> +    PlacesUtils.history.clear().then(() => {

ditto

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placesTitleNoUpdate.js
@@ +108,5 @@
>      });
>    }
>  
>    function waitForClearHistory(aCallback) {
> +    PlacesUtils.history.clear().then(aCallback);

ditto (I'd completely remove the helper and inline promiseClearHistory.then)

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_placestitle.js
@@ +15,5 @@
>    function waitForCleanup(aCallback) {
>      // delete all cookies
>      cm.removeAll();
>      // delete all history items
> +    PlacesUtils.history.clear().then(aCallback);

ditto

::: services/sync/tests/unit/test_history_engine.js
@@ +14,5 @@
>  Service.engineManager.clear();
>  
> +function clearHistory() {
> +  let cb = Async.makeSpinningCallback();
> +  PlacesUtils.history.clear().then(cb, cb);

promiseClearHistory

well I guess the idea is clear, I won't comment further about it to avoid useless spamming

::: services/sync/tests/unit/test_history_store.js
@@ +61,5 @@
>    return function() {
>      try {
>        func.apply(this, arguments);
>      } catch (ex) {
> +      PlacesUtils.history.clear();

this should probably wait... not sure.

::: toolkit/components/places/tests/unit/test_history_clear.js
@@ +20,5 @@
>  
>    run_next_test();
>  }
>  
> +add_task(function test_history_clear()

function*

@@ +69,5 @@
>    yield promiseAsyncUpdates();
>  
> +  // Clear history.
> +  let expired = promiseTopicObserved(PlacesUtils.TOPIC_EXPIRATION_FINISHED);
> +  yield Promise.all([PlacesUtils.history.clear(), expired]);

can't we just use promiseClearHistory?

looks like you wrongly removed the promiseOnClearHistoryObserved test from here?
Likely should be 

yield Promise.all([ promiseClearHistory(), promiseOnClearHistoryObserved ]);

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1113178-Add-PlacesTestUtils.clearHistory.patch

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1113178-Replace-removeAllPages-in-services-sync-.patch

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1113178-Replace-removeAllPages-in-browser-base-c.patch

Fix tests in browser/base/content/test/general/.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1113178-Replace-removeAllPages-in-toolkit-compon.patch

Fix tests in toolkit/components/jsdownloads/.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0005-Bug-1113178-Replace-removeAllPages-in-toolkit-compon.patch

Fix tests in toolkit/components/social/.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0006-Bug-1113178-Replace-removeAllPages-in-browser-compon.patch

Fix tests in browser/components/places/.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0007-Bug-1113178-Replace-removeAllPages-in-toolkit-compon.patch

Fix tests in toolkit/components/places/.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0008-Bug-1113178-Fix-all-remaining-tests-using-removeAllP.patch

Fix remaining tests.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f1bdeef05bf

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8550736 [details] [diff] [review]
0003-Bug-1113178-Replace-removeAllPages-in-browser-base-c.patch

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

::: browser/base/content/test/general/browser_sanitizeDialog.js
@@ +980,2 @@
>  
> +  let deleteDownloads = Task.spawn(function deleteAllDownloads() {

function*

@@ +1001,5 @@
> +      }
> +    });
> +  });
> +
> +  return Promise.all([clearHistory, deleteDownloads, updateFormHistory]);

nit: could directly put PTU.clearHistory here

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 8550744 [details] [diff] [review]
0007-Bug-1113178-Replace-removeAllPages-in-toolkit-compon.patch

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

::: toolkit/components/places/tests/expiration/test_clearHistory.js
@@ +128,5 @@
>    // Expire all visits for the bookmarks.  This does the same thing as the
> +  // PlacesTestUtils.clearHistory() helper, but it is made explicit here
> +  // because History.clear() is the function we are testing.
> +  let topic = PlacesUtils.TOPIC_EXPIRATION_FINISHED;
> +  yield Promise.all([PlacesUtils.history.clear(), promiseTopicObserved(topic)]);

I think it's not worth to bring this on, let's just do a simple 

// Expire all visits for the bookmarks
yield PlacesUtils.history.clear();

::: toolkit/components/places/tests/expiration/test_notifications.js
@@ +24,5 @@
>  function run_test() {
>    // Set interval to a large value so we don't expire on it.
>    setInterval(3600); // 1h
>  
> +  PlacesTestUtils.clearHistory().then(check_result);

the intent of the test was to check that we were not firing more than one notification, thus the 2s timeout.

By changing it like this it will just test that we fire a notification.

I guess there's no better way than a timeout to check thing don't happen...

::: toolkit/components/places/tests/unit/test_history_clear.js
@@ +20,5 @@
>  
>    run_next_test();
>  }
>  
> +add_task(function test_history_clear()

function*

@@ +68,5 @@
>    ]);
>    yield promiseAsyncUpdates();
>  
> +  // Clear history.
> +  yield PlacesTestUtils.clearHistory();

this test was checking whether onClearHistory was invoked, any reason to remove that check?
we can't rely on the expiration topic forwarding, if we plan to remove that forwarding.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8550746 [details] [diff] [review]
0008-Bug-1113178-Fix-all-remaining-tests-using-removeAllP.patch

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

::: browser/base/content/test/newtab/head.js
@@ +241,5 @@
>    });
>  }
>  
>  function clearHistory(aCallback) {
> +  PlacesTestUtils.clearHistory().then(aCallback);

I assume you didn't remove this helper cause it would require too many boring changes... maybe worth to file a mentored bug to do that?

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #18)
> ::: browser/base/content/test/newtab/head.js
> >  function clearHistory(aCallback) {
> > +  PlacesTestUtils.clearHistory().then(aCallback);
> 
> I assume you didn't remove this helper cause it would require too many
> boring changes... maybe worth to file a mentored bug to do that?

Hmmm it's actually only used in head.js. Should've checked :) Removed the helper function.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #17)
> ::: toolkit/components/places/tests/expiration/test_notifications.js
> @@ +24,5 @@
> >  function run_test() {
> >    // Set interval to a large value so we don't expire on it.
> >    setInterval(3600); // 1h
> >  
> > +  PlacesTestUtils.clearHistory().then(check_result);
> 
> the intent of the test was to check that we were not firing more than one
> notification, thus the 2s timeout.
> 
> By changing it like this it will just test that we fire a notification.

Ah, right. I should've read the test description more thoroughly. Re-added the timeout.

> ::: toolkit/components/places/tests/unit/test_history_clear.js
> @@ +68,5 @@
> >    ]);
> >    yield promiseAsyncUpdates();
> >  
> > +  // Clear history.
> > +  yield PlacesTestUtils.clearHistory();
> 
> this test was checking whether onClearHistory was invoked, any reason to
> remove that check?
> we can't rely on the expiration topic forwarding, if we plan to remove that
> forwarding.

Reverted to just replace removeAllPages() with PlacesUtils.history.clear().

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/9ff2b7687317
https://hg.mozilla.org/integration/fx-team/rev/ff8d3068e1ac
https://hg.mozilla.org/integration/fx-team/rev/ee7e5d9917b8
https://hg.mozilla.org/integration/fx-team/rev/c2748e129e98
https://hg.mozilla.org/integration/fx-team/rev/43bce807be2e
https://hg.mozilla.org/integration/fx-team/rev/85ad09aef80d
https://hg.mozilla.org/integration/fx-team/rev/110a765e46a2

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9ff2b7687317
https://hg.mozilla.org/mozilla-central/rev/ff8d3068e1ac
https://hg.mozilla.org/mozilla-central/rev/ee7e5d9917b8
https://hg.mozilla.org/mozilla-central/rev/c2748e129e98
https://hg.mozilla.org/mozilla-central/rev/43bce807be2e
https://hg.mozilla.org/mozilla-central/rev/85ad09aef80d
https://hg.mozilla.org/mozilla-central/rev/110a765e46a2

Comment 23

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8549797 [details] [diff] [review]
0002-Bug-1113178-Replace-removeAllPages-in-services-sync-.patch

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

Pity about ensureThrows() but even so it LGTM

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Thanks!

https://hg.mozilla.org/integration/fx-team/rev/af8aa104c175

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/af8aa104c175