Closed Bug 1448081 Opened 6 years ago Closed 6 years ago

Replace remaining unit test usages of updatePlaces with PlacesTestUtils.addVisits

Categories

(Toolkit :: Places, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: standard8, Assigned: sreeise, Mentored)

References

Details

(Whiteboard: [fxsearch][lang=js])

Attachments

(1 file, 9 obsolete files)

16.65 KB, patch
standard8
: review+
Details | Diff | Splinter Review
We've got a few instances of .updatePlaces in test files that we should replace:

https://searchfox.org/mozilla-central/search?q=.updatePlaces(&case=false&regexp=false&path=browser%2F**test
https://searchfox.org/mozilla-central/search?q=.updatePlaces(&case=false&regexp=false&path=browser%2F**test
https://searchfox.org/mozilla-central/search?q=.updatePlaces(&case=false&regexp=false&path=toolkit%2Fcomponents%2Fplaces%2Ftests%2Funit

These should be replaced by the new async function PlacesTestUtils.addVisits:

https://searchfox.org/mozilla-central/rev/c217fbde244344fedfd07b57a740c694a456dbca/toolkit/components/places/tests/PlacesTestUtils.jsm#17-35

Note the function is async, whereas the old one is callback based. Though in most places, there's a new Promise wrapper that can be removed, allowing the new function to be called direct.

I'm happy to mentor this, if there's questions please ask.
Blocks: 1448041
(In reply to Mark Banner (:standard8) from comment #0)
> We've got a few instances of .updatePlaces in test files that we should
> replace:
> 
> https://searchfox.org/mozilla-central/search?q=.
> updatePlaces(&case=false&regexp=false&path=browser%2F**test
> https://searchfox.org/mozilla-central/search?q=.
> updatePlaces(&case=false&regexp=false&path=browser%2F**test
> https://searchfox.org/mozilla-central/search?q=.
> updatePlaces(&case=false&regexp=false&path=toolkit%2Fcomponents%2Fplaces%2Fte
> sts%2Funit
> 
> These should be replaced by the new async function PlacesTestUtils.addVisits:
> 
> https://searchfox.org/mozilla-central/rev/
> c217fbde244344fedfd07b57a740c694a456dbca/toolkit/components/places/tests/
> PlacesTestUtils.jsm#17-35
> 
> Note the function is async, whereas the old one is callback based. Though in
> most places, there's a new Promise wrapper that can be removed, allowing the
> new function to be called direct.
> 
> I'm happy to mentor this, if there's questions please ask.

Hey Mark,

I would like to work on this, and I don't mind at all if you mentor me on this, that would be awesome. I want to ask a couple questions. I can see it specifies to use either href, url, or nsIURI and then there are optional areas. I am trying to understand what the addVisits is doing exactly. Is this for updating the history/bookmarks/downloads and adding the places that have been visited or is there one specific area it is like 'History' that it is for? I want to make sure I understand how it is used to better understand how to fix this. Thanks.
Flags: needinfo?(standard8)
(In reply to Sean Reeise from comment #1)
> I would like to work on this, and I don't mind at all if you mentor me on
> this, that would be awesome.

Great.

> I want to ask a couple questions. I can see it
> specifies to use either href, url, or nsIURI and then there are optional
> areas. I am trying to understand what the addVisits is doing exactly. Is
> this for updating the history/bookmarks/downloads and adding the places that
> have been visited or is there one specific area it is like 'History' that it
> is for? I want to make sure I understand how it is used to better understand
> how to fix this. Thanks.

PlacesTestUtils.addVisits is a function that is used only for tests. What it does is to insert history visits into the database so that we can simulate having visited various sites and run various tests based on those.

updatePlaces is the older API that isn't as nice as addVisits as it is callback based, rather than async based.

Note that since addVisits takes a variety of parameters, we don't generally need to wrap urls in Netutil.newURI or Services.io.newURI etc, e.g. https://searchfox.org/mozilla-central/rev/de5c4376b89c3603341d226a65acea12f8851ec5/toolkit/components/places/tests/unit/test_download_history.js#13,178-184 - though sometimes that wrapping is needed for other functions / checks.

Also, I didn't mention it earlier, once you've done a build, you can generally run tests with:

./mach test path/to/test
Flags: needinfo?(standard8)
Hey Mark,

Sorry it took so long to reply. I wanted to get your feedback on the text attachment. This is for the first link at browser/base/content/test/sanitize/browser_sanitize-timespans.js. It is the setupHistory() function. First, I imported PlacesTestUtils.jsm at the top of the page right below the other const variables like this:

const {PlacesTestUtils} =
  ChromeUtils.import("resource://testing-common/PlacesTestUtils.jsm", {});

Note: I wasn't sure about the import statement - is this correct? 
The attachment is what I changed in the setupHistory() function. I built the changes and ran the tests using ./mach test for browser_sanitize-timespans.js and all tests passed no issues.

Is this about what should be done here, or am I missing anything on how to make these changes?
Flags: needinfo?(standard8)
(In reply to Sean Reeise from comment #3)
> Sorry it took so long to reply. I wanted to get your feedback on the text
> attachment.

No problem. Note, if you attach diffs, or push to mozreview it is generally easier for me to comment on with some context.

(https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html)

> First, I imported PlacesTestUtils.jsm at the top of
> the page right below the other const variables like this:
> 
> const {PlacesTestUtils} =
>   ChromeUtils.import("resource://testing-common/PlacesTestUtils.jsm", {});

Yes, that's right. Though you could possibly do it before the const variables (we tend to get imports first and then do var/const global definitions).

> The attachment is what I changed in the setupHistory() function. I built the
> changes and ran the tests using ./mach test for
> browser_sanitize-timespans.js and all tests passed no issues.

That's strange, I made the changes locally and I'm seeing test failures, did you run "./mach test browser_sanitize-timespans.js" or "./mach test browser/base/content/test/sanitize/browser_sanitize-timespans.js" ?

The first failure I see is:

  FAIL Pretend visit to 10minutes.com should now be deleted - 
Stack trace:
chrome://mochitests/content/browser/browser/base/content/test/sanitize/browser_sanitize-timespans.js:onHistoryReady:97

The issue here is the addPlace sub-function, you need to remove the visits array and change 'transitionType' to 'transition':

    places.push({
      uri: aURI,
      title: aTitle,
      visitDate: aVisitDate,
      transition: Ci.nsINavHistoryService.TRANSITION_LINK
    });

You can also drop the makeURI wrappers in the function calls to addPlace since addVisits handles that.

Also don't forget to re-indent the function, since we normally use 2-space indents.
Flags: needinfo?(standard8)
This replaces updatePlaces with PlacesTestUtils.addVisits in browser_sanitize-timespans.js
Attachment #8962026 - Attachment is obsolete: true
(In reply to Mark Banner (:standard8) from comment #4)
> No problem. Note, if you attach diffs, or push to mozreview it is generally
> easier for me to comment on with some context.

Ah I wasn't thinking about importing the changes, I uploaded it as a patch and removed the makeURI parts.


> Yes, that's right. Though you could possibly do it before the const
> variables (we tend to get imports first and then do var/const global
> definitions).

Gotcha, thanks I was not sure on that, I moved it above the var/const declarations.

> The first failure I see is:
> 
>   FAIL Pretend visit to 10minutes.com should now be deleted - 
> Stack trace:
> chrome://mochitests/content/browser/browser/base/content/test/sanitize/
> browser_sanitize-timespans.js:onHistoryReady:97
> 
> The issue here is the addPlace sub-function, you need to remove the visits
> array and change 'transitionType' to 'transition':
> 
>     places.push({
>       uri: aURI,
>       title: aTitle,
>       visitDate: aVisitDate,
>       transition: Ci.nsINavHistoryService.TRANSITION_LINK
>     });

I am a little confused on this part, I thought the attachment I added before changed it to 'transition' instead of 'transitionType'. This is probably part of not doing it through a patch, sorry about that.

> Also don't forget to re-indent the function, since we normally use 2-space
> indents.

Re-indent the addPlace function?

Hopefully this partial patch will be easier to see where I made the changes, thanks for the help Mark. I will continue working on the other parts to this bug and put them all in one commit, and will let you know if I run into a problem as well.
Flags: needinfo?(standard8)
(In reply to Sean Reeise from comment #6)
> Ah I wasn't thinking about importing the changes, I uploaded it as a patch
> and removed the makeURI parts.

Great, thanks.

> I am a little confused on this part, I thought the attachment I added before
> changed it to 'transition' instead of 'transitionType'. This is probably
> part of not doing it through a patch, sorry about that.

Sorry that was my fault, I'd missed that when I was copying & pasting. The patch is much better :-)

> > Also don't forget to re-indent the function, since we normally use 2-space
> > indents.
> 
> Re-indent the addPlace function?

All of setupHistory please. Since you removed the `return new Promise(resolve => {` block, all of it needs to be two spaces less. Also, please make sure the properties in the places.push argument are aligned as well.

> I will continue working on the other parts to this
> bug and put them all in one commit, and will let you know if I run into a
> problem as well.

Yes, that's fine. Thank you for working on this.
Assignee: nobody → reeisesean
Flags: needinfo?(standard8)
Added replacements browser_library_downloads.js

In order to get the Downloads folder selected I switched it to await promiseLibrary() - I was not able to get it to work using the same way as before. Based on reading the way the Downloads folder was opening before, I figured this will work pretty much the same way. The tests are the same, it is just the way the downloads folder opens that is different.
Hey Mark,

I added a patch that has replacements in browser_library_downloads.js. I tested it on Linux and Windows and both passed the tests for Downloads being opened and the URI matching. Additionally, I was not sure whether to keep the VisitInfo function because it worked regardless of the function being there and using Ci.nsINavHistoryService.TRANSITION_*, but in the patch I used PlacesUtils.history.TRANSITION_* because that is what it tests for. I am thinking that could also mean I did not do it correctly so I wanted to be sure. Is this how opening and checking the Downloads folder should be implemented?

Also in test_454977.js I am not sure how to resolve the visitId. In order for the test to run all the way through and check both unhidden and hidden visits, the visitId must be resolved in the test currently. When changing to PlacesTestUtils.addVisits I am not sure how to to get the visitId from the addVisits, or another place. Could you possibly point me in the right direction of how to correctly get the visitId? I can add the visits and pass the tests that check for visits in the checkResults() function but because retrieving the placId from the database relies on whether visitId is greater than 0, it will not test for boolean values using Assert.equals().
Flags: needinfo?(standard8)
Attachment #8962664 - Attachment is obsolete: true
Flags: needinfo?(standard8)
Comment on attachment 8964278 [details] [diff] [review]
Parial patch - browser_library_downloads.js and browser_sanitize-timespans.js

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

Thank you for working on this, there's some comments below. Additionally looking at your changes on browser_library_downloads.js has revealed an existing issue with the test (explained below), so it will be great to get that fixed properly.

Re test_454977.js I'm still thinking about that at the moment.

::: browser/components/places/tests/browser/browser_library_downloads.js
@@ +25,5 @@
> +  function addPlace(aURI, aTransitionType) {
> +    places.push({
> +      uri: aURI,
> +      transition: aTransitionType,
> +      visitDate: VisitInfo(aTransitionType)

We can drop visitDate since we don't need to supply it if we want to just use `now`, since addVisits sets that for us.

https://hg.mozilla.org/mozilla-central/log/tip/toolkit/components/places/tests/PlacesTestUtils.jsm

@@ +34,5 @@
> +  addPlace("http://mozilla.org", PlacesUtils.history.TRANSITION_TYPED);
> +  addPlace("http://google.com", PlacesUtils.history.TRANSITION_DOWNLOAD);
> +  addPlace("http://en.wikipedia.org", PlacesUtils.history.TRANSITION_TYPED);
> +  addPlace("http://ubuntu.org", PlacesUtils.history.TRANSITION_DOWNLOAD);
> +  await PlacesTestUtils.addVisits(places);

I think since addPlace is quite simple, in this case we can just use the format of:

await PlacesTestUtils.addVisits([{
  uri: "http://mozilla.org",
  transition: PlacesUtils.history.TRANSITION_TYPED,
}, {
  ...
}]);

@@ +36,5 @@
> +  addPlace("http://en.wikipedia.org", PlacesUtils.history.TRANSITION_TYPED);
> +  addPlace("http://ubuntu.org", PlacesUtils.history.TRANSITION_DOWNLOAD);
> +  await PlacesTestUtils.addVisits(places);
> +
> +  library = await promiseLibrary("Downloads");

If you move the registerCleanupFunction to just after this, then you can make this `let library = ... ` and avoid the global.

@@ +42,2 @@
>          // Make sure Downloads is present.
> +  isnot(library.PlacesOrganizer._places.selectedNode, null,

Please switch this to use `Assert.notEqual`, it gives better debug output.

@@ +44,1 @@
>                "Downloads is present and selected");

Is this a diff -w ? The indentation here looks weird. Just a normal patch is fine for now.

@@ +45,5 @@
>  
>  
>          // Check results.
>          let testURIs = ["http://ubuntu.org/", "http://google.com/"];
> +  for (let element of library.ContentArea.currentView

The issue with the test is here, we're not always letting the library fully fill out the entries before we continue with the test.

Running with the current state on my machine, I found that it isn't actually checking any URLs.

There's two problems here, though I think they can be solved with one change.

Before the for loop we need to add another line, to wait for the entries to be full:

await BrowserTestUtils.waitForCondition(() =>
  library.ContentArea.currentView.associatedElement.children.length == testURIs.length);

This serves two purposes: 1) Making sure the downloads window has filled the list with the expected number of items, 2) making sure we will check all the items that are in testURIs.

Either way, it'll warn us if there are too few entries, or if any of them are missing. That's good enough for this test.

@@ +50,2 @@
>                                             .associatedElement.children) {
>            is(element._shell.download.source.url, testURIs.shift(),

Please make this `Assert.equal`
(In reply to Sean Reeise from comment #9)
> Also in test_454977.js I am not sure how to resolve the visitId. In order
> for the test to run all the way through and check both unhidden and hidden
> visits, the visitId must be resolved in the test currently. When changing to
> PlacesTestUtils.addVisits I am not sure how to to get the visitId from the
> addVisits, or another place.

You can use PlacesTestUtils.waitForNotification("onVisits", ...) to get the visit id.
(In reply to Mark Banner (:standard8) from comment #10)
> Comment on attachment 8964278 [details] [diff] [review]
> Parial patch - browser_library_downloads.js and browser_sanitize-timespans.js
> 
> Review of attachment 8964278 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for working on this, there's some comments below. Additionally
> looking at your changes on browser_library_downloads.js has revealed an
> existing issue with the test (explained below), so it will be great to get
> that fixed properly.

No problem at all, and I was thinking something looked a little off, so this makes more sense.

> 
> Re test_454977.js I'm still thinking about that at the moment.
> 
> ::: browser/components/places/tests/browser/browser_library_downloads.js
> @@ +25,5 @@
> > +  function addPlace(aURI, aTransitionType) {
> > +    places.push({
> > +      uri: aURI,
> > +      transition: aTransitionType,
> > +      visitDate: VisitInfo(aTransitionType)
> 
> We can drop visitDate since we don't need to supply it if we want to just
> use `now`, since addVisits sets that for us.

Sounds good.

> https://hg.mozilla.org/mozilla-central/log/tip/toolkit/components/places/
> tests/PlacesTestUtils.jsm
> 
> @@ +34,5 @@
> > +  addPlace("http://mozilla.org", PlacesUtils.history.TRANSITION_TYPED);
> > +  addPlace("http://google.com", PlacesUtils.history.TRANSITION_DOWNLOAD);
> > +  addPlace("http://en.wikipedia.org", PlacesUtils.history.TRANSITION_TYPED);
> > +  addPlace("http://ubuntu.org", PlacesUtils.history.TRANSITION_DOWNLOAD);
> > +  await PlacesTestUtils.addVisits(places);
> 
> I think since addPlace is quite simple, in this case we can just use the
> format of:
> 
> await PlacesTestUtils.addVisits([{
>   uri: "http://mozilla.org",
>   transition: PlacesUtils.history.TRANSITION_TYPED,
> }, {
>   ...
> }]);

Yes I was wondering about this, but I wasn't sure whether to change it based on the VisitInfo function, thank you.

> @@ +36,5 @@
> > +  addPlace("http://en.wikipedia.org", PlacesUtils.history.TRANSITION_TYPED);
> > +  addPlace("http://ubuntu.org", PlacesUtils.history.TRANSITION_DOWNLOAD);
> > +  await PlacesTestUtils.addVisits(places);
> > +
> > +  library = await promiseLibrary("Downloads");
> 
> If you move the registerCleanupFunction to just after this, then you can
> make this `let library = ... ` and avoid the global.
> 
> @@ +42,2 @@
> >          // Make sure Downloads is present.
> > +  isnot(library.PlacesOrganizer._places.selectedNode, null,
> 
> Please switch this to use `Assert.notEqual`, it gives better debug output.

Gotcha, I will change this as well.

> 
> @@ +44,1 @@
> >                "Downloads is present and selected");
> 
> Is this a diff -w ? The indentation here looks weird. Just a normal patch is
> fine for now.


I am honestly not sure what happened with the patch, this is my fault, sorry about that. I will make sure it is fixed in the next one.


> @@ +45,5 @@
> >  
> >  
> >          // Check results.
> >          let testURIs = ["http://ubuntu.org/", "http://google.com/"];
> > +  for (let element of library.ContentArea.currentView
> 
> The issue with the test is here, we're not always letting the library fully
> fill out the entries before we continue with the test.
> 
> Running with the current state on my machine, I found that it isn't actually
> checking any URLs.
> 
> There's two problems here, though I think they can be solved with one change.
> 
> Before the for loop we need to add another line, to wait for the entries to
> be full:
> 
> await BrowserTestUtils.waitForCondition(() =>
>   library.ContentArea.currentView.associatedElement.children.length ==
> testURIs.length);
> 
> This serves two purposes: 1) Making sure the downloads window has filled the
> list with the expected number of items, 2) making sure we will check all the
> items that are in testURIs.
> 
> Either way, it'll warn us if there are too few entries, or if any of them
> are missing. That's good enough for this test.


This makes a lot more sense, thank you. I appreciate your help on this.


> @@ +50,2 @@
> >                                             .associatedElement.children) {
> >            is(element._shell.download.source.url, testURIs.shift(),
> 
> Please make this `Assert.equal`


Gotcha, I will change this as well.
(In reply to Marco Bonardo [::mak] from comment #11)
> (In reply to Sean Reeise from comment #9)
> > Also in test_454977.js I am not sure how to resolve the visitId. In order
> > for the test to run all the way through and check both unhidden and hidden
> > visits, the visitId must be resolved in the test currently. When changing to
> > PlacesTestUtils.addVisits I am not sure how to to get the visitId from the
> > addVisits, or another place.
> 
> You can use PlacesTestUtils.waitForNotification("onVisits", ...) to get the
> visit id.

Thank you Marco, this helps a lot and saves me a lot of time. I will make the change here too.
Replaced tests using updatePlaces with PlacesTestUtils.addVisits from PlacesTestUtils.jsm.
Replaced in:
browser/base/content/test/sanitize/browser_sanitize-timespans.js
browser/components/places/tests/browser/browser_library_downloads.js
toolkit/components/places/tests/unit/test_454977.js
toolkit/components/places/tests/unit/test_isURIVisited.js
Replaced tests using updatePlaces with PlacesTestUtils.addVisits from PlacesTestUtils.jsm.
Replaced in:
browser/base/content/test/sanitize/browser_sanitize-timespans.js
browser/components/places/tests/browser/browser_library_downloads.js
toolkit/components/places/tests/unit/test_454977.js
toolkit/components/places/tests/unit/test_isURIVisited.js
Attachment #8966021 - Attachment is obsolete: true
This last attachment has all of the changes. I accidentally configured bzexport with the wrong email and had to resubmit the patch, sorry about that. 

In test_isURIVisited.js, in the original it is going through the callback regardless of errors, which would normally happen with updatePlaces when reaching "about:" false in the schemes when reaching the handleError() function. Those cant be added to visits so I am assuming this is intentional to test the isURIVisited api? Currently I just changed the callback to a function and removed the handle error/result/completion functions, and used .then and .catch to recall the callback function. Not sure if this is what we are suppose to do, or if this should be implemented in a different way.
Flags: needinfo?(standard8)
Comment on attachment 8964278 [details] [diff] [review]
Parial patch - browser_library_downloads.js and browser_sanitize-timespans.js

Just to note, bzexport should let you obsolete existing patches when you upload new ones.
Attachment #8964278 - Attachment is obsolete: true
Flags: needinfo?(standard8)
Comment on attachment 8966024 [details] [diff] [review]
Replaced tests using updatePlaces with PlaceTestUtils.addVisits

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

Thank you for the updated patch, we're heading in the right direction. Note also, there's a test_download_history.js to do as well, though that looks a bit simpler.

Next time feel free to set the review or feedback flag to :standard8. That should flag me automatically - you should be able to do this from bzexport as well, iirc.

::: browser/components/places/tests/browser/browser_library_downloads.js
@@ +30,5 @@
> +    await library.close();
> +    await PlacesUtils.history.clear();
> +  });
> +
> +  let library = await promiseLibrary("Downloads");

This should still be moved before the registerCleanupFunction call, so that library is logically declared before it is used.

::: toolkit/components/places/tests/unit/test_454977.js
@@ +10,5 @@
>  // Returns the Place ID corresponding to an added visit.
>  async function task_add_visit(aURI, aVisitType) {
> +  // Wait for a visits notification and get the visitId.
> +  let visitId;
> +  PlacesTestUtils.waitForNotification("onVisits", visits => {

You should assign this to a variable as it returns a promise, e.g. let visitsPromise = ...

Then after the addVisits, add a line for `await visitsPromise`. That will ensure the notification has been received and the visitId has definitely been received.

::: toolkit/components/places/tests/unit/test_isURIVisited.js
@@ +29,5 @@
>    gRunner = step();
>    gRunner.next();
>  }
>  
>  function* step() {

So this test could do with a little bit of rework to handle async better.

First step is to drop the run_test function, drop gRunner completely and change this step() to be a:

add_task(async function test_isURIVisited() {

@@ +39,5 @@
>      for (let t in PlacesUtils.history.TRANSITIONS) {
>        info("With transition " + t);
>        let transition = PlacesUtils.history.TRANSITIONS[t];
>  
>        let uri = NetUtil.newURI(scheme + "mozilla.org/");

You should be able to change NetUtil.newURI to Services.io.newURI.

@@ +41,5 @@
>        let transition = PlacesUtils.history.TRANSITIONS[t];
>  
>        let uri = NetUtil.newURI(scheme + "mozilla.org/");
>  
> +      history.isURIVisited(uri, async function(aURI, aIsVisited) {

The async callback won't quite work here, as isURIVisited isn't expecting it.

What's slightly better to do is:

let [receivedURI, visited] = await new Promise(resolve => {
  history.isURVisited(uri, (receivedUri, visited) => {
    resolve(receivedURI, visited);
  });
});

You can then continue with the Assert.ok etc.

@@ +51,2 @@
>  
> +          history.isURIVisited(uri, function(aURI2, aIsVisited2) {

Given the extra isURIVisited checks, it may be worth making what I said above into a function, so that you can do:

[receivedURI, visited] = await isURIVisited(uri);

@@ +65,3 @@
>  
> +        await PlacesTestUtils.addVisits(uri, transition)
> +          .then(() => callback())

No need for the .then/.catch here. You can simply do the functions in the callback after the await, e.g. (in sudo code):

await addVisits();
await isURIVisited();
await history.clear();
await isURIVisited();

@@ +69,2 @@
>        });
>        yield undefined;

This yield can be dropped as well as the do_test_finished() with the above rewrites.
test_download_history.js

When using PlacesTestUtils I am getting an error that looks like it is from the last part of addVisits:

Stack Trace:
0:03.54 pid:146852 JavaScript error: , line 0: uncaught exception: Ensure history has been updated and is visible to read-only connections - threw exception: Error: Connection is not open.

This seems like it could possibly be from the connection being closed from addDownloads but I am not entirely sure. I have tried differnt variations of the function, but have not come to a solution yet. All of the tests pass, but this is going to throw an error regardless. I am still working on this one and wanted to see if you had any thoughts on what could be done here? 


test_454977.js

I changed the function to use await visitsPromise after the visits have been added but there is an issue. When the transition is TRANSITION_EMBED, calling await visitsPromise for PlacesTestUtils.waitForNotificaiton will cause the test to stop and eventually timeout. So in the patch, I changed it to call await visitPromise and gets the visitId as long as the transition is not TRANSITION_EMBED. Not exactly sure why it does this, but I believe it has something to do with the visits being hidden? If you want to reproduce, remove the if block and just use 'await visitsPromise'.

Another option would be to change the first and last 'task_add_visit' calls to just add visits right then. Either way, when adding TRANSITION_EMBED, we don't do anything with visit_count and trying to check for visitId using
waitForNotification would stall out. So technically with the first and last task_add_visit calls all we are doing is calling addVisits for those regardless of where we call the function.


test_isURIVisited.js

The function for testing the isURIVisited api returns an array with the uri object and visited boolean. With these, we can successfully check the results the same way as before. However, when using PlacesTestUtils.addVisits, it will throw and error with certain type of URI's trying to be added. So in the patch I catch the error and use Assert.ok to make sure the error message is from addVisits and not anywhere else since we are just testing the isURIVisited api, and we would already know that those URI's will throw an error anway.


browser_library_downloads.js

Fixed the placement of where the 'library' is initialized.
Attachment #8968068 - Flags: review?(standard8)
Attachment #8966024 - Attachment is obsolete: true
I just realized I didn't change transitionType to transition in test_download_history.js. The test outcome is the same with this fixed, and I will include it as well with the next patch.
This patch has more changes for test_download_history.js. I changed it to use add_task and async await.  I was having issues with 'connection not open' for PlacesTestUtils like I mentioned in the previous patch, so I tried a couple different things and noticed that when changed to async await, PlacesUtils.history.clear() would no longer work within the callback function itself.  I attemtped to use 'await new Promise' and then resolve with history.clear().then(resolve) like test_dh_addBookmarkRemovedDownload is doing, but the same timeout issues occur:
 
ERROR Unexpected exception Ensure history has been updated and is visible to read-only connections - timed out after 50 tries.

So I changed it to resolve the visit id and referrer id and then do a last Assertion test. At the end I cleared out history with PlacesUtils but I had to remove it from the the Promise. I am not sure it makes much of a difference, except for where clean up is neccesary. The tests passed with correct URI's and the issues with connection are gone.

There may be a solution im not seeing, so I wanted to get some feedback on the updated patch.
Attachment #8968517 - Flags: feedback?(standard8)
Attachment #8968068 - Attachment is obsolete: true
Attachment #8968068 - Flags: review?(standard8)
Comment on attachment 8968517 [details] [diff] [review]
Replaced tests using updatePlaces with PlaceTestUtils.addVisits

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

Sorry for the delay in responding. Definitely heading in the right direction. I think I've answered your questions below.

::: toolkit/components/places/tests/unit/test_454977.js
@@ +21,5 @@
> +    uri: aURI,
> +    transition: aVisitType
> +  }]);
> +
> +  if (aVisitType != TRANSITION_EMBED) {

This is fine, I just tested without the patch, and in the TRANSITION_EMBED case, the visitId is 0, which is then skipped in the checks below.

::: toolkit/components/places/tests/unit/test_download_history.js
@@ +155,5 @@
>  });
>  
> +add_task(async function test_dh_addDownload_referrer() {
> +  let downloadsPromise = new Promise(resolve => {
> +    waitForOnVisit(function DHAD_prepareReferrer(aURI, aVisitID) {

You're definitely heading in the right direction here. 

What would be slightly better to do is two things:

1) waitForOnVisit can be replaced by `PlacesTestUtils.waitForNotification("onVisits", ...`);

There's an existing example here: https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js#45

2) You should then be able to drop this promise, I think, and simplify the test a bit:

```
let referrerPromise = PlacesTestUtils.waitForNotification(...);

await PlacesTestUtils.addVisits(...);

await referrerPromise;

let downloadPromise = PlacesTestUtils.waitForNotification(...);

gDownloadHistory.addDownload(...);

await downloadPromise;

// Final checks
Assert.equal(...);

await PlacesUtils.history.clear();
```

Along the way you'll need to save the referrer visit ID and the referring ID for the checks.

::: toolkit/components/places/tests/unit/test_isURIVisited.js
@@ +49,3 @@
>  
> +      try {
> +        await PlacesTestUtils.addVisits([{

Rather than the try catch, please can you wrap this in an if statement:

if (PlacesUtils.history.canAddURI(aURI)) {
  ...
}

@@ +61,4 @@
>  
> +      let [receivedURI2, visited2] = await visitsPromise(aURI);
> +      Assert.ok(aURI.equals(receivedURI2));
> +      Assert.ok(SCHEMES[scheme] ? visited2 : !visited2);

Please can you make this:

Assert.equal(SCHEMES[scheme], visited2);

That should make it a little clearer as to what it is testing.
Attachment #8968517 - Flags: feedback?(standard8) → feedback+
This has the changes for test_download_history, and the other changes from the last review. Thanks for showing me the browser_multi_redirect_frecency.js file, it helped a good bit. Question, should waitForNotification in test_454977.js return uri.equals(aURI) similar to how that file does it?
Attachment #8969170 - Flags: feedback?(standard8)
Attachment #8968517 - Attachment is obsolete: true
Comment on attachment 8969170 [details] [diff] [review]
Changes for test_download_history.js

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

Thanks Sean, this is looking good. Just a couple of minor things. For the next one, request review as I think we're pretty much there now.

::: toolkit/components/places/tests/unit/test_454977.js
@@ +12,5 @@
> +  // Wait for a visits notification and get the visitId.
> +  let visitId;
> +  let visitsPromise = PlacesTestUtils.waitForNotification("onVisits", visits => {
> +    visitId = visits[0].visitId;
> +    return true;

> Question, should waitForNotification in test_454977.js return uri.equals(aURI) similar to how that file does it?

Yes, I think it should, just to confirm we get the right notification.

::: toolkit/components/places/tests/unit/test_download_history.js
@@ +159,5 @@
> +  let visitId;
> +  let referrerPromise = PlacesTestUtils.waitForNotification("onVisits", visits => {
> +    visitId = visits[0].visitId;
> +    let {uri} = visits[0];
> +    Assert.ok(uri.equals(REFERRER_URI));

No need to Assert the uri.equals here, as the promise just won't resolved if it isn't received.

@@ +178,5 @@
> +  let referrerId;
> +  let downloadPromise = PlacesTestUtils.waitForNotification("onVisits", visits => {
> +    referrerId = visits[0].referrerId;
> +    let {uri} = visits[0];
> +    Assert.ok(uri.equals(DOWNLOAD_URI));

ditto wrt "No need to Assert the uri.equals here, as the promise just won't resolved if it isn't received."
Attachment #8969170 - Flags: feedback?(standard8) → feedback+
Contains all changes for bug 1448081 for review.
Attachment #8969396 - Flags: review?(standard8)
Attachment #8969170 - Attachment is obsolete: true
Contains all changes for bug 1448081 for review, and removed blank line from test_454977.js from last patch.
Attachment #8969402 - Flags: review?(standard8)
Attachment #8969396 - Attachment is obsolete: true
Attachment #8969396 - Flags: review?(standard8)
Comment on attachment 8969402 [details] [diff] [review]
Replaced tests using updatePlaces with PlaceTestUtils.addVisits

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

Great, thank you for the updates, and for keeping going on this. r=Standard8

I'll get this landed for you.
Attachment #8969402 - Flags: review?(standard8) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c339f571d35
Replaced tests using updatePlaces with PlaceTestUtils.addVisits. r=Standard8
https://hg.mozilla.org/mozilla-central/rev/9c339f571d35
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Backed out changeset 9c339f571d35 (bug 1448081) for browser chrome failures on browser/components/places/tests/browser/browser_library_downloads.js. a=backout

Log of the failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=174883906&repo=autoland&lineNumber=2819

  INFO - Console message: [JavaScript Error: "getScreenshot(http://mozilla.org/) failed: Unix error 2 during operation open on file /var/folders/sf/my4n_2117tq9ztcc942qs9lc00000w/T/tmpS8FnZH.mozrunner/thumbnails/bde098adf84ba902f3af189ac6b7c299.png (No such file or directory)" {file: "resource://activity-stream/lib/Screenshots.jsm" line: 76}]
17:08:28     INFO - getScreenshotForURL@resource://activity-stream/lib/Screenshots.jsm:76:7
17:08:28     INFO - async*maybeCacheScreenshot@resource://activity-stream/lib/Screenshots.jsm:130:32
17:08:28     INFO - async*_fetchScreenshot@resource://activity-stream/lib/TopSitesFeed.jsm:226:11
17:08:28     INFO - async*_fetchIcon@resource://activity-stream/lib/TopSitesFeed.jsm:214:11
17:08:28     INFO - async*getLinksWithDefaults@resource://activity-stream/lib/TopSitesFeed.jsm:159:11
17:08:28     INFO - async*refresh@resource://activity-stream/lib/TopSitesFeed.jsm:175:25
17:08:28     INFO - async*onAction@resource://activity-stream/lib/TopSitesFeed.jsm:398:9
17:08:28     INFO - _middleware/</<@resource://activity-stream/lib/Store.jsm:51:11
17:08:28     INFO - Store/this[method]@resource://activity-stream/lib/Store.jsm:29:55
17:08:28     INFO - customDispatch/this.placesChangedTimer<@resource://activity-stream/lib/PlacesFeed.jsm:205:11
17:08:28     INFO - 
17:08:28     INFO - Buffered messages finished
17:08:28     INFO - TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_library_downloads.js | Uncaught exception - undefined - timed out after 50 tries.
17:08:28     INFO - Leaving test bound test
17:08:28     INFO - GECKO(1942) | MEMORY STAT | vsize 4523MB | residentFast 509MB | heapAllocated 122MB
17:08:28     INFO - TEST-OK | browser/components/places/tests/browser/browser_library_downloads.js | took 5804ms
17:08:28     INFO - checking window state
17:08:28     INFO - TEST-START | browser/components/places/tests/browser/browser_library_infoBox.js
17:08:28     INFO - GECKO(1942) | MEMORY STAT | vsize 4525MB | residentFast 511MB | heapAllocated 128MB
17:08:28     INFO - TEST-OK | browser/components/places/tests/browser/browser_library_infoBox.js | took 604ms
17:08:28     INFO - checking window state
17:08:28     INFO - TEST-START | browser/components/places/tests/browser/browser_library_left_pane_middleclick.js
17:08:29     INFO - GECKO(1942) | MEMORY STAT | vsize 4539MB | residentFast 518MB | heapAllocated 138MB
17:08:29     INFO - TEST-OK | browser/components/places/tests/browser/browser_library_left_pane_middleclick.js | took 669ms

Backout:
https://hg.mozilla.org/mozilla-central/rev/d36d2c2ab772c4b5479e2516a87e830cab8da509
Status: RESOLVED → REOPENED
Flags: needinfo?(reeisesean)
Resolution: FIXED → ---
Dorel: I really don't think this should have been backed out. It had landed fine on inbound & central. It wasn't until after it merged across to autoland that a problem was flagged. That means it is more likely something on autoland that caused the issue - it may not have been obvious & you might not have been able to find it which I'm happy with, but it would be nice to have seen this stated on the backout message.


As it is, the test has actually been made more strict - previously it was possible that not all the expected urls would be present.

The true regressing point for this updated test is bug 1455737 - https://hg.mozilla.org/integration/autoland/rev/40c5d3600d35, I've just tried applying that locally and it causes the issue.

Ursula/Ed, please can you take a look at this?
Flags: needinfo?(usarracini)
Flags: needinfo?(reeisesean)
Flags: needinfo?(edilee)
Ah yup, this seems to be exactly what Paolo was referring to in bug 1455737 comment 8, which is now backed out. Sorry for landing it too quickly and causing this backout! At least now we know there's a test that would have caught the brokenness -- unfortunately it happened to land on separate integration branches :(
Flags: needinfo?(usarracini)
Flags: needinfo?(edilee)
The backout of bug 1455737 was backed out probably because bug 1433230 also landed in the meantime.
Depends on: 1455737
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6747ccc87604
Replaced tests using updatePlaces with PlaceTestUtils.addVisits. r=Standard8
https://hg.mozilla.org/mozilla-central/rev/6747ccc87604
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Thank you for re-pushing this Ed.

Sean: Thank you for working on this.
You need to log in before you can comment on or make changes to this bug.