Fix various "undefined property" errors raised in places tests

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla55
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

In working on other bugs, I've been hitting various javascript errors when running the places tests of the form:

(pid:11625) "JavaScript strict warning: resource://gre/modules/Bookmarks.jsm, line 1947: ReferenceError: reference to undefined property "source""

I'd like to get some of these cleaned up, so that it is easier to know if my patches are introducing more issues or not.
See Also: → 1300416

Comment 2

2 years ago
mozreview-review
Comment on attachment 8870091 [details]
Bug 1366829 - Fix various 'undefined property' errors raised in places tests.

https://reviewboard.mozilla.org/r/141578/#review145410

I agree with Kit that some of these should not warn at all, and that's a js bug. Though, after 9 months we didn't see any action in the filed bug, and these are annoying, so let's just fix them and hope in the future we can revert back to more compact code.

::: toolkit/components/places/Bookmarks.jsm:790
(Diff revision 1)
>     *       may be overwritten.
>     */
>    fetch(guidOrInfo, onResult = null, options = {}) {
> +    if (!("concurrent" in options)) {
> +      options.concurrent = false;
> +    }

does this really warn? I thought that using options.concurrent in a bool condition didn't issue a warning.

::: toolkit/components/places/Bookmarks.jsm:2174
(Diff revision 1)
>        delta: syncChangeDelta,
>        parent: parentId,
>        start_index: startIndex,
>        item_type: Bookmarks.TYPE_SEPARATOR
>      });
>  }

why did you remove the final newline on the file? I think it's required by coding style.

::: toolkit/components/places/tests/unit/test_hosts_triggers.js:87
(Diff revision 1)
>  
>  add_task(async function test_moz_hosts_update() {
>    let places = [];
>    urls.forEach(function(url) {
>      let place = { uri: url.uri,
> -                  title: "test for " + url.url,
> +                  title: "test for " + url.uri,

url.uri.spec

::: toolkit/components/places/tests/unit/test_promiseBookmarksTree.js:173
(Diff revision 1)
>  async function test_promiseBookmarksTreeForEachNode(aNode, aOptions, aExcludedGuids) {
>    Assert.ok(aNode.bookmarkGuid && aNode.bookmarkGuid.length > 0);
>    let item = await PlacesUtils.promiseBookmarksTree(aNode.bookmarkGuid, aOptions);
>    await compareToNode(item, aNode, true, aExcludedGuids);
>  
> -  for (let i = 0; i < aNode.childCount; i++) {
> +  let count = ("childCount" in aNode ? aNode.childCount : 0);

Let's instead change this to

if (!PlacesUtils.nodeIsContainer(aNode))
  return item;

for ...
Attachment #8870091 - Flags: review?(mak77) → review+
Via MattN and Kit's bug, is this "strict mode" or javascript.options.strict=true? ie, https://lists.mozilla.org/pipermail/dev-platform/2014-May/004486.html
(In reply to Mark Hammond [:markh] from comment #3)
> Via MattN and Kit's bug, is this "strict mode" or
> javascript.options.strict=true? ie,
> https://lists.mozilla.org/pipermail/dev-platform/2014-May/004486.html

"use strict" is on for Bookmarks.jsm, but not for the other files afaict. I don't think we should turn these off - some of the locations I wasn't sending the right info, and this alerted me to it.
Assignee

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8870091 [details]
Bug 1366829 - Fix various 'undefined property' errors raised in places tests.

https://reviewboard.mozilla.org/r/141578/#review145410

> does this really warn? I thought that using options.concurrent in a bool condition didn't issue a warning.

It seems to:

"JavaScript strict warning: resource://gre/modules/Bookmarks.jsm, line 832: ReferenceError: reference to undefined property "concurrent""

When running `toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js` and quite a few others.

> why did you remove the final newline on the file? I think it's required by coding style.

This isn't removing the final newline - currently there are two there, so this makes it just one (ESlint would have triggered on eol-last here).
Comment hidden (mozreview-request)

Comment 7

2 years ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7fbfa19e0d73
Fix various 'undefined property' errors raised in places tests. r=mak

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7fbfa19e0d73
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Duplicate of this bug: 1333309
You need to log in before you can comment on or make changes to this bug.