Closed Bug 1366829 Opened 3 years ago Closed 3 years ago

Fix various "undefined property" errors raised in places tests

Categories

(Toolkit :: Places, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

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 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.
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).
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7fbfa19e0d73
Fix various 'undefined property' errors raised in places tests. r=mak
https://hg.mozilla.org/mozilla-central/rev/7fbfa19e0d73
Status: NEW → RESOLVED
Closed: 3 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.