949614 - is() should use ===

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This causes about 3500 failures right now: <https://tbpl.mozilla.org/?tree=Try&rev=59a49e9c1911>. Will attach the failures in a bit.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Failures

Comment 2

[Boris Zbarsky [:bzbarsky]]

Andrea, test_url_empty_port.html seems to be testing that "" == 0 for port.  That expression tests true in JS, but I really doubt that's what the test is meaning to test...

Comment 3

[Jeff Walden [:Waldo]]

Even better than ===, Object.is (equates NaNs, doesn't equate -0 and +0).  I've wanted to do the same thing for the xpcshell checking methods, too, but as you see it's a long slog to fix.

Comment 4

[Andrea Marchesini [:baku]]

I'm fixing this bug here: Bug 930450

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Consolidate some spellchecking test code

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Use === for SimpleTest.is

This is more likely to be correct, and a necessary step in case we ever want
to move to Object.is.

This keeps ise as an alias for is, and introduces is_loosely for the old
behaviour.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8580260 [details] [diff] [review]
Use === for SimpleTest.is

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

It goes without saying that you should feel free to parcel out these changes into separate revisions as far as you want, to lock in what's good while letting what's tricky move at its own pace.

::: accessible/tests/mochitest/attributes.js
@@ +142,5 @@
>  
>    var errorMsg = " for " + aID + " at offset " + aOffset;
>  
> +  is_loosely(startOffset.value, aStartOffset, "Wrong start offset" + errorMsg);
> +  is_loosely(endOffset.value, aEndOffset, "Wrong end offset" + errorMsg);

I think you can use |is| here if you change this code in accessible/tests/mochitest/common.js:

    case "textAttrs": {
      var prevOffset = -1;
      for (var offset in accTree[prop]) {
        if (prevOffset !=- 1) {
          var attrs = accTree[prop][prevOffset];
          testTextAttrs(acc, prevOffset, attrs, { }, prevOffset, offset, true);
        }
        prevOffset = offset;
      }

      if (prevOffset != -1) {
        var charCount = getAccessible(acc, [nsIAccessibleText]).characterCount;
        var attrs = accTree[prop][prevOffset];
        testTextAttrs(acc, prevOffset, attrs, { }, prevOffset, charCount, true);
      }

      break;
    }

by putting a + in front of |offset| in that first call to testTextAttrs.  If that doesn't work, please fix this to not is_loosely in a followup.

::: accessible/tests/mochitest/events.js
@@ +1729,5 @@
>      is(aEvent.start, this.startOffset,
>         "Wrong start offset for " + prettyName(aID));
>      is(aEvent.length, modifiedTextLen, "Wrong length for " + prettyName(aID));
>      var changeInfo = (aIsInserted ? "inserted" : "removed");
> +    is_loosely(aEvent.isInserted, aIsInserted,

I don't think this change is needed, so long as you change this code in accessible/tests/mochitest/events/test_text_alg.html from using 0/1 to using false/true:

    const kRemoval = 0;
    const kInsertion = 1;

@@ +1838,5 @@
>  
>      if (!event)
>        return;
>  
> +    is_loosely(event.isExtraState, aIsExtraState,

I think you can leave this alone if you change accessible/tests/mochitest/states.js to have

const kOrdinalState = false;

instead of

const kOrdinalState = 0;

::: accessible/tests/mochitest/events/test_tree.xul
@@ +261,5 @@
>        [
>          new nameChangeChecker("invalidateRow: ", 1, 0),
>          new nameChangeChecker("invalidateRow: ", 1, 1),
>          new rowNameChangeChecker("invalidateRow: ", 1),
> +        new treeInvalidatedChecker("invalidateRow", 1, 1, null, undefined)

I don't think either of these changes are desirable.  The problem appears to be that inside treeInvalidatedChecker, we have this code:

        try {
          var startCol = propBag.getPropertyAsInt32("startcolumn");
        } catch (e if e.name == 'NS_ERROR_NOT_AVAILABLE') {
          startCol = null;
        }
        is(startCol, aStartCol,
           "Wrong 'startcolumn' of 'treeInvalidated' event on " + aMsg);

That code's all fine.  It defaults startCol to null if the getPropertyAsInt32 throws.  But look at what we have for aEndCol!

        try {
          var endCol = propBag.getPropertyAsInt32("endcolumn");
        } catch (e if e.name == 'NS_ERROR_NOT_AVAILABLE') {
          startCol = null;
        }
        is(endCol, aEndCol,
           "Wrong 'endcolumn' of 'treeInvalidated' event on " + aMsg);

The catch-code looks pretty clearly like it should be setting |endCol| -- pure copypasta.

So change that instead of this.

::: accessible/tests/mochitest/table/test_sels_listbox.xul
@@ +81,1 @@
>             aId + ": no columns should be selected");

So if you look at the callers of this method, *every* single one passes |aSelIdexesArray === null|.  So you can remove the if-test and else-block.

But...I think someone misunderstood how size_is in XPIDL is normally used.  The normal way to call a method taking an out length and out array/retval/size_is(out length) is to call the method but not pass an outparam for the array-out argument, then look at the array through the return value, like so.
 
      // getSelectedColumns
      var selColsCount = {};
      var colIndices = aAcc.getSelectedColumnIndices(selColsCount);
      // colIndices is an array of column indexes, selColsCount.value is its length

In fact, per CallMethodHelper::GatherAndConvertResults() it looks like retval outparam arguments aren't even touched, to have a .value added to them.  So this code compares null/undefined against a non-existent property, which is rather pointless.  So really, you should convert this to:

      // getSelectedColumns
      var selColsCount = {};
      var colIndices = aAcc.getSelectedColumnIndices(selColsCount);

      is(selColsCount.value, aSelCount,
         aId + ": wrong number of selected columns");
      is(colIndices.length, aSelCount,
         aId + ": wrong number of selected columns");

@@ +120,1 @@
>             aId + ": no row should be selected");

The same size_is XPIDL issues are present here, too.  Please convert this as follows:

      // getSelectedRows
      var selRowsCount = {};
      var rowIndices = aAcc.getSelectedRowIndices(selRowsCount);

      is(selRowsCount.value, aSelCount,
         aId + ": wrong number of selected rows");

      if (!aSelIndexesArray) {
        is(rowIndices, 0,
           aId + ": no row should be selected");
      } else {
        for (var i = 0; i < rowIndices.length; i++) {
          is(rowIndices[i], aSelIndexesArray[i],
             aId + ": wrong selected row index " + i);
        }
      }

This of course assumes that when |aSelIndexesArray === null|, an empty array really truly is returned.  Try before you buy.

Also: before, this code happened not to implode because |i < selCols.length| was |0 < undefined| on first iteration, so no testing happened at all.  (!)  Extra reason to be careful making the change.

@@ +158,5 @@
>        is(selColsCount.value, aSelCount,
>           aId + ": wrong number of selected cells");
>  
>        if (!aSelIndexesArray) {
> +        is(selCols.value, undefined,

Same set of changes as for the row-code above needed, same reasons and all.  Just make sure to rename stuff away from row to cell (just as the current code refers to columns but really means cells).  :-\

::: caps/tests/mochitest/test_disableScript.xul
@@ +76,5 @@
>      let deferred = Promise.defer();
>      // We need to append 'name' to avoid running afoul of recursive frame detection.
>      let frameURI = uri + "?name=" + name;
>      navigateFrame(ifr, frameURI).then(function() {
> +      is(String(ifr.contentWindow.location), frameURI, "Successful load");

I'd be inclined to do ifr.contentWindow.location.href instead.

::: docshell/test/navigation/test_bug430723.html
@@ +58,5 @@
>    };
>  }
>  
>  var step1 =function() {
> +  window.is(String(testWindow.location), gTallRedBoxURI, "Ensure red page loaded.");

Hmm, there are probably a lot of comparisons of .location to a string.  s/\.location/\.location\.href/g then.

::: dom/apps/tests/test_packaged_app_common.js
@@ +171,2 @@
>      }
>      if (aExpectedApp.short_name) {

The function checkInstalledApp in dom/apps/tests/test_packaged_app_install.html appears to be unused.  Please remove it?

@@ +175,5 @@
>        }
>        is(aApp.updateManifest.short_name, aExpectedApp.short_name, "Check short name mini-manifest");
>      }
>      if (aApp.manifest) {
> +      is_loosely(aApp.manifest.version, aVersion, "Check version");

It looks to me like |aApp.manifest.version| for our packaged tests probably always a string, like "2" or "3.6" or "37.0.1" or whatever.  So |aVersion| should always be a string.  But many callers pass in whole numbers instead right now.  Please change them to strings in a followup.  This is the edited list of all of them, cut down to the places that need fixing, I think:

[jwalden@find-waldo-now src]$ ack checkAppState ../../dom/apps/tests/
../../dom/apps/tests/test_signed_pkg_install.html
137:        PackagedTestHelper.checkAppState(gApp, 1, expected,
236:        PackagedTestHelper.checkAppState(gApp, 1, expected,

../../dom/apps/tests/test_import_export.html
232:      PackagedTestHelper.checkAppState(PackagedTestHelper.gApp, 2, expected,

../../dom/apps/tests/test_packaged_app_install.html
74:    PackagedTestHelper.checkAppState(evt.application, aVersion, aExpectedApp,
243:        PackagedTestHelper.checkAppState(PackagedTestHelper.gApp, 2, expected,
330:        PackagedTestHelper.checkAppState(PackagedTestHelper.gApp, 3, expected,
348:        PackagedTestHelper.checkAppState(PackagedTestHelper.gApp, 3, expected,

Also this:

../../dom/apps/tests/test_packaged_app_update.html
73:  PackagedTestHelper.checkAppState(PackagedTestHelper.gApp, aExpectedVersion,

which corresponds to, I think:

175:        checkLastAppState.bind(undefined, miniManifestURL, false, false,
176:                               2, PackagedTestHelper.next);

235:    updateApp(true, 2, 3);

254:    updateApp(true, 3, 4);

261:    updateApp(false, 4, 4, true);

Basically I think all the literal numbers in the above are the exact things that need changing, to make them strings.

::: dom/base/test/test_XHRSendData.html
@@ +231,5 @@
>        is(xhr.getResponseHeader("Result-Content-Type"), null);
>      }
>  
>      if (test.resContentLength) {
> +      is(xhr.getResponseHeader("Result-Content-Length"), String(test.resContentLength), "Wrong Content-Length sent");

This is getting long enough to triple-line it, IMO.

::: dom/base/test/test_bug346485.html
@@ +26,5 @@
>   */
>  
>  var o = document.getElementById('o');
>  
> +is(String(o.htmlFor), 'a b',

Would have seemed preferable to me to have

function checkHtmlFor(htmlFor, arr)
{
  var len = htmlFor.length;
  is(len, arr.length, htmlFor + ": incorrect htmlFor length");
  for (var i = 0; i < len; i++)
    is(htmlFor[i], arr[i], htmlFor + ": wrong element at " + i);
}

but I guess this is what was happening before.  Maybe a followup tweak or something?  Or if these are definitely supposed to be testing stringification, then add .toString() to them.

::: dom/base/test/test_domrequesthelper.xul
@@ +47,2 @@
>        is(dummy._messages, undefined, "Messages is undefined");
> +      is_loosely(dummy._window, undefined, "Window is undefined");

is(..., null, ...) for these

I think you can remove the _messages test.  The only two hits for the string in this file are these two in this patch.  There are no hits for it in DOMRequestHelper.jsm.  It looks dead to me.  Get someone who's written this code to r= the reomval.

@@ +63,2 @@
>        is(dummy._messages, undefined, "Messages is undefined");
> +      is_loosely(dummy._window, undefined, "Window is undefined");

is(..., null, ...) for these

@@ +387,5 @@
>          var req_ = dummy.getRequest(id);
>          is(req, req_, "Got correct request");
>          dummy.removeRequest(id);
>          req = dummy.getRequest(id);
> +        is(req, undefined, "No request");

Any chance you could make a followup or something to make this check for null, after making DOMRequestHelper.jsm explicitly return null rather than fall off the end of its function?

::: dom/base/test/test_url.html
@@ +310,5 @@
>  
>      url = new URL("http://localhost/");
>      url.host = "[2001::1]:30";
>      is(url.hostname, "[2001::1]", "IPv6 hostname");
> +    is(url.port, "30", "Port");

...URL.prototype.port returns a string, not a number?  :-\  That seems a bit strange to me.

::: dom/bindings/test/test_dom_xrays.html
@@ +43,2 @@
>        } else {
> +        is_loosely(obj[name], value, "Should get the right value for \"" + name + "\" through Xrays");

File a bug to change this to is().  I can't think why loose comparisons have any value here, except as transition.

::: dom/browser-element/mochitest/browserElementTestHelpers.js
@@ +212,5 @@
>          // otherwise we'll expect /every/ priority/LRU change to match
>          // expectedPriority/expectedLRU.
>          observed = true;
>  
> +        is(lru, String(expectedLRU),

Don't do this -- please change

dom/browser-element/mochitest/priority/test_ForegroundLRU.html
55:      expectPriorityWithLRUSet(childID, 'FOREGROUND', 1)
64:    var p = expectPriorityWithLRUSet(childID, 'FOREGROUND', 0)

to pass strings instead.

::: dom/browser-element/mochitest/browserElement_ReloadPostRequest.js
@@ +99,5 @@
>        is(e.detail.buttons[1].message, 'cancel');
>        is(e.detail.message, expectedMessage.message);
>        is(e.detail.buttons.length, 2);
>        is(e.detail.showCheckbox, false);
> +      is(e.detail.checkMessage, undefined);

That's a strange value for a property of an Event to have.  I looked a little deeper -- are we 100% certain this shouldn't be checkboxMessage?  Get a browser-element person to confirm (pun intended), but I very much suspect this is a bug in the test.

::: dom/contacts/tests/test_contacts_basics.html
@@ +149,5 @@
>        checkContacts(createResult1, properties1);
>        // Some manual testing. Testint the testfunctions
>        // tel: [{type: ["work"], value: "123456", carrier: "testCarrier"} , {type: ["home", "fax"], value: "[phone]"}],
>        is(findResult1.tel[0].carrier, "testCarrier", "Same Carrier");
> +      is(String(findResult1.tel[0].type), "work", "Same type");

Based on the comment above, can this be tel[0].type[0] instead of an explicit coercion?

@@ +157,5 @@
>  
>        is(findResult1.adr[0].countryName, "country 1", "Same country");
>  
>        // email: [{type: ["work"], value: "[e-mail]"}]
> +      is(String(findResult1.email[0].type), "work", "Same Type");

.type[0]?

::: dom/contacts/tests/test_contacts_basics2.html
@@ +229,5 @@
>      req.onsuccess = function () {
>        ok(cloned.id, "The contact now has an ID.");
>        is(cloned.email[0].value, "new email!", "Same Email");
>        isnot(createResult1.email[0].value, cloned.email[0].value, "Clone has different email");
> +      is(String(cloned.givenName), "Tom", "New Name");

Would [0] work here as well?

::: dom/events/test/test_bug689564.html
@@ +30,5 @@
>     "div should have an onscroll handler");
>  
>  div.setAttribute("onpopstate", "div");
>  is(window.onpopstate, null, "div should not forward onpopstate");
> +is(div.onpopstate, undefined, "div should not have onpopstate handler");

is("onpopstate" in div, false, ...) would be a better test, if it works.  Please do that if it does.

::: dom/events/test/test_dragstart.html
@@ +167,5 @@
>    is(dt.types.length, 0, "invalid type setData");
>    is(dt.getData(""), "", "invalid type getData"),
>    dt.mozSetDataAt("", "Invalid Type", 0);
>    is(dt.types.length, 0, "invalid type setDataAt");
> +  is(dt.mozGetDataAt("", 0), undefined, "invalid type getDataAt"),

So I think this happens to be correct according to the code:

void
DataTransfer::MozGetDataAt(JSContext* aCx, const nsAString& aFormat,
                           uint32_t aIndex,
                           JS::MutableHandle<JS::Value> aRetval,
                           mozilla::ErrorResult& aRv)
{
  nsCOMPtr<nsIVariant> data;
  aRv = MozGetDataAt(aFormat, aIndex, getter_AddRefs(data));
  if (aRv.Failed()) {
    return;
  }

  if (!data) {
    return; // we would end up here, not having set aRetval, leaving its prior value
  }

But I'm not certain it's what was *supposed* to happen.  https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer#mozGetDataAt.28.29 documents semantics as "Retrieves the data associated with the given format for an item at the specified index, or null if it does not exist."  Note how it says null should be returned here, right?

So this should have a followup to fix mozGetDataAt, or to fix the documentation, and perhaps to adjust this test further.

@@ +506,1 @@
>         testid + " getDataAt " + expectedtypes[f]);

By my skimming expecteddata[f] is always truthy, so I don't understand that ternary.  And it's not obvious to me why a loose comparison is needed for left-hand-side purposes.  Please file a followup to make this more precise about whatever it's doing.

::: dom/html/test/forms/test_input_number_mouse_events.html
@@ +52,5 @@
>    input.value = 0;
>  
>    // Test click on spin-up button:
>    synthesizeMouse(input, SPIN_UP_X, SPIN_UP_Y, { type: "mousedown" });
> +  is(input.value, "1", "Test step-up on mousedown on spin-up button");

By the right-hand rule, I hope?

::: dom/html/test/forms/test_output_element.html
@@ +105,5 @@
>  }
>  
>  function checkHtmlForIDLAttribute(element)
>  {
> +  is(String(element.htmlFor), 'a b',

Same checkHtmlFor comment as on the other test.

::: dom/html/test/forms/test_step_attribute.html
@@ +446,4 @@
>        checkValidity(input, true, apply);
>  
>        input.value = '1.5';
> +      is(input.value, '2', "check that the value changes to the nearest valid step, choosing the higher step if both are equally close");

But but round to nearest even is the only rounding mode evar111!11!cos(0)!

::: dom/html/test/forms/test_valueasdate_attribute.html
@@ +187,5 @@
>      element.value = data[0];
> +    if (data[1]) {
> +      is(String(element.valueAsDate), "Invalid Date",
> +         "valueAsDate should return null "  +
> +         "when the element value is not a valid date");

This message is wrong, something like s/return null/be an invalid Date object/.

::: dom/html/test/test_bug332893-3.html
@@ +44,5 @@
>                
>  	var form1 = document.getElementById("form1");
>  	var form2 = document.getElementById("form2");
>  
> +	is(form1.elements.length, 2, "Form 1 has the correct length");

wat @ someone comparing a length to the corresponding string in this file o_O

::: dom/html/test/test_documentAll.html
@@ +30,5 @@
>  p = document.getElementById("content");
>  
>  // Test that several elements with the same id or name behave correctly
>  function testNumSame() {
> +  is(document.all.id0, undefined, "no ids");

Ugh.  The previous code may well have been *assuming* that same-value semantics were in use for this testing function, because namedItem("id0") would return null here in marked contrast, despite getter behavior being declared to work through the namedItem method.  Progress, at least.

Could you add

  is(document.all.namedItem("id0"), null, "no ids");

next to this to highlight the behavioral difference, and to ensure it's explicitly tested in our tree?  (Probably is, but no sense taking chances.)

@@ +78,5 @@
>  id3list = document.all.id3;
>  rC(child[3]);
>  is(id3list.length, 1, "now one length");
>  rC(child[5]);
> +is(document.all.id3, undefined, "now none");

Also a sibling namedItem test, please.

@@ +134,5 @@
>      is(document.all[nameval], e, "should have name");
>      hasName.shift();
>    }
>    else {
> +    is(document.all[nameval], undefined, "shouldn't have name");

And again.

::: dom/indexedDB/test/unit/test_multientry.js
@@ +89,5 @@
>        is(req.result.primaryKey, test.indexes[j].k, "found expected index primary key at index " + j + testName);
>        req.result.continue();
>      }
>      e = yield undefined;
> +    ok(req.result == null, "exhausted indexes");

Why not is(req.result, null, ...)?

@@ +101,5 @@
>        is(req.result.primaryKey, test.indexes[j].k, "found expected temp index primary key at index " + j + testName);
>        req.result.continue();
>      }
>      e = yield undefined;
> +    ok(req.result == null, "exhausted temp index");

And same here.

@@ +185,5 @@
>        is(req.result.primaryKey, indexes[j].k, "found expected index primary key at index " + j + testName);
>        req.result.continue();
>      }
>      e = yield undefined;
> +    ok(req.result == null, "exhausted indexes");

And same.

@@ +197,5 @@
>        is(req.result.primaryKey, indexes[j].k, "found expected temp index primary key at index " + j + testName);
>        req.result.continue();
>      }
>      e = yield undefined;
> +    ok(req.result == null, "exhausted temp index");

Same.

::: dom/indexedDB/test/unit/test_setVersion_abort.js
@@ +45,5 @@
>  
>    event = yield undefined;
>    is(event.type, "abort", "Got transaction abort event");
>    is(event.target, transaction, "Right target");
> +  is(event.target.transaction, undefined, "No transaction");

I'm not sure I believe this.  Why would a transaction have a .transaction property?  khuey doesn't believe it either -- and he said just remove this line, which was what I was thinking, so do that.

..and beyond that, several lines further down, is code similar to this where event.target is a request, suggesting this is bad copypasta.  So yeah.  Remove this line.

::: dom/indexedDB/test/unit/test_setVersion_events.js
@@ +39,1 @@
>      is(event.target, db1, "Correct target");

So if |event.target| is an IDBDatabase, and IDBDatabase has no source property, and it inherits from EventTarget only (which just adds in add/removeEventListener and dispatchEvent)...why should there be a .source property here?  My guess is this is just comparing a non-existent property, i.e. undefined, against null.

So this test probably should be removed.  Same for the other two changed lines in this file.  Confirm with khuey or someone who knows indexeddb before going ahead with this, as usual.

::: dom/media/test/test_audio2.html
@@ +20,5 @@
>      return;
>    manager.started(token);
>    var a1 = new Audio(test.name);
>    is(a1.getAttribute("preload"), "auto", "Preload automatically set to auto");
> +  is(String(a1.src.match("/[^/]+$")), "/" + test.name, "src OK");

This is better written

ok(a1.src.endsWith("/" + test.name), "src OK");

::: dom/plugins/test/mochitest/test_npruntime_identifiers.html
@@ +33,5 @@
>        is(reflector[prop], prop, "Property " + prop);
>      }
>  
> +    for (i = -10; i < 0; ++i) {
> +      is(reflector[i], String(i), "Property " + i);

Dollars to donuts says this test, had it been a string-versus-string test all along, would have been broken awhile back, before jsid of -1 changed from being an integer-valued jsid to being a string-valued jsid.  All the better for us, I guess.

::: dom/plugins/test/mochitest/test_propertyAndMethod.html
@@ +33,5 @@
>          delete pluginProto.propertyAndMethod;
>          ok(isNaN(plugin.propertyAndMethod + 0), "Shouldn't be set yet!");
>  
>          plugin.propertyAndMethod = 5;
> +        is(+plugin.propertyAndMethod, 5, "Should be set to 5!");

I confess to much confusion as to why this is necessary.  The test plugin does INT32_TO_NPVARIANT(5, *result), which I would really really really have thought would caused the getter to return the number value 5.  I understand that a non-primitive value would be wrapped in an NPObject and such and be weird, but I didn't think that applied to primitives.

Please rope in some plugin person to edumacate me here.

::: dom/tests/mochitest/webapps/test_install_app.xul
@@ +54,5 @@
>      ok(app.installTime + fuzzySpan >= beforehand, "install time is after install() call");
>      ok(app.installTime <= Date.now() + fuzzySpan, "install time is before install success");
>      is(app.manifestURL, url, "manifest URL");
>      is(app.manifest.name, "Basic App", "manifest.name");
> +    is(String(app.manifest.installs_allowed_from), "*",

Split this into

    is(app.manifest.installs_allowed_from.length, 1, "allowed-from length must be 1");
    is(app.manifest.installs_allowed_from[0], "*", "allowed from anywhere");

rather than invoke stringification code.

::: dom/tests/mochitest/webapps/test_launch_paths.xul
@@ +54,5 @@
>      ok(app.installTime + fuzzySpan >= beforehand, "install time is after install() call");
>      ok(app.installTime <= Date.now() + fuzzySpan, "install time is before install success");
>      is(app.manifestURL, url, "manifest URL");
>      is(app.manifest.name, "Basic App with Launch Paths", "manifest.name");
> +    is(String(app.manifest.installs_allowed_from), "*",

is(app.manifest.installs_allowed_from.length, 1, ...);
is(app.manifest.installs_allowed_from[0], "*");

as requested elsewhere.

::: editor/libeditor/tests/spellcheck.js
@@ +9,5 @@
>      return false;
>    }
>  
>    for (var i = 0; i < numWords; ++i) {
> +    var word = String(sel.getRangeAt(i));

Preference for explicit .toString() instead of hazier conversion semantics.

::: editor/libeditor/tests/test_bug1100966.html
@@ +38,5 @@
>  
>        var sel = getSpellCheckSelection();
>        is(sel.rangeCount, 2, "We should have two misspelled words");
> +      is(String(sel.getRangeAt(0)), "fivee", "Correct misspelled word");
> +      is(String(sel.getRangeAt(1)), "sixx", "Correct misspelled word");

Same .toString() preference.

::: editor/libeditor/tests/test_bug596333.html
@@ +79,5 @@
>      append(" workd");
>      edit.blur();
>      onSpellCheck(edit, function () {
>        gMisspeltWords.push("workd");
> +      ok(isSpellingCheckOk(getEditor(), gMisspeltWords),

Why are you passing any arguments to isSpellingCheckOk?  It doesn't take any.  Maybe it should, and not use global mutable state, but that should be separately changed, and not pointlessly halfway like here.

::: js/xpconnect/tests/chrome/test_bug571849.xul
@@ +30,1 @@
>      ok(/Text/.test(output[1]), "the 0th element was a text node");

Instead of

    var output;

let's instead do

    var index, value;
    for (let i in Iterator(docnodes))
        index = i[0], value = i[1];
    is(index, 0, ...);
    ok(/Text/.test(value), ...);

for more readability.

::: js/xpconnect/tests/mochitest/test_bug691059.html
@@ +41,1 @@
>         "prototype.onmouseleave should be settable, but the value should stay null");

Uh, these are just bizarre.  Of *course* setting Document.prototype.onmouseenter has no effect on Document.onmouseenter.  They're totally different things.

Two things to change here.  First, use

  is("onmouseenter" in obj, false, ...);

to capture the real test, which is that there's no property (not allowing a property with value undefined, or an accessor returning such, to be a head-fake).  Second, use something like

"setting <Interface>.prototype.onmouseenter has no effect on the non-existent <Interface>.onmouseenter"

instead of the current message that would blatantly contradict the test as you've written it.

::: layout/style/test/test_flexbox_flex_grow_and_shrink.html
@@ +77,2 @@
>  advance_clock(1000);
> +is(+cs.flexGrow, 10, "flexGrowTwoToThree at 1.5s");

For all the is(...) tests you're changing, don't coerce the computed-style property -- compare against a string instead.  Adding in an extra coercion means more code to execute to make the comparison, that might possibly be broken at some time or other in ways that make the test start spuriously succeeding or so.  In contrast is(cs.flexGrow, "10", ...) is not vulnerable to such fallout.

The is_approx tests do make sense to do as you've done them, tho.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +291,5 @@
>      SimpleTest.ok(pass, name, diag);
>  };
>  
>  /**
> + * Roughly equivalent to ok(a===, name)

ok(a == b, name)

::: toolkit/components/microformats/tests/test_Microformats.html
@@ +252,5 @@
>    origdate = Microformats.iso8601FromDate(jsdate, true);
>    is(dateCal.dtstart, origdate, "date round trip");
>  
>    var dateCal = new hCalendar(document.getElementById("vcal_vcard"));
> +  is(String(dateCal.description), "Mozilla's Birthday", "vcard in vcal");

Add the same sort of isOneElementArrayContaining helper and use that in all this file's changes.

::: toolkit/components/microformats/tests/test_Microformats_hCalendar.html
@@ +229,5 @@
>    is(hcalendar.summary, "Bad Movie Night - Gigli (blame mike spiegelman)", "06-component-vevent-uri-relative - summary");
>    is(hcalendar.dtstart, "2006-01-15T00:00:00", "06-component-vevent-uri-relative - dtstart");
>  
>    hcalendar = new hCalendar(document.getElementById("07-component-vevent-description-simple"));
> +  is(String(hcalendar.description), "Project xyz Review Meeting Minutes", "07-component-vevent-description-simple - description");

Add a helper

function isOneElementArrayContaining(arr, contents, msg)
{
  is(arr.length, 1, msg + ": length must be 1");
  is(arr[0], contents, msg + ": must contain expected value");
}

Then use that all three locations in this test.  Please have a microformats person review this, tho -- I couldn't be 100% certain from specs that the description item was supposed to be a list.

::: toolkit/components/microformats/tests/test_Microformats_hCard.html
@@ +575,5 @@
>  
>    hcard = new hCard(document.getElementById("01-tantek-basic"));
>  
>    is(hcard.fn, "Tantek Çelik", "01-tantek-basic - fn");
> +  is(String(hcard.url), "http://tantek.com/", "01-tantek-basic - url");

...okay, this file *really* wants that helper function, rather than casting arrays to strings a hundred times or so.

::: toolkit/components/url-classifier/tests/mochitest/classifiedAnnotatedFrame.html
@@ +59,5 @@
>        nodeMatch |=
>          (blockedTrackingNodes[i] == document.getElementById(badids[j]));
>      }
>  
>      allNodeMatch &= nodeMatch;

Could you change this to allNodeMatch = allNodeMatch && nodeMatch?  And same in the loop below.

::: toolkit/content/tests/chrome/findbar_window.xul
@@ +450,2 @@
>            "Currently highlighted match should be at " + aTest.current);
> +        is(aMatches[2], String(aTest.total),

Mild preferences for using .toString() to convert numbers to strings, than to use String().  (Honestly I guess this applied to the entire patch, I just didn't recognize it early enough.  Don't feel pressured to go back and fix the entire patch, that's too much.  :-) )

@@ +501,5 @@
>                        .getService(Ci.nsIPrefBranch);
>        prefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 0);
>  
>        enterStringIntoFindField("t");
> +      is(String(gBrowser.contentWindow.getSelection()), "T", "First T should be selected.");

.toString() to invoke selection-specific conversion code, please.

@@ +506,4 @@
>  
>        prefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 1);
>        setTimeout(function() {
> +        is(String(gBrowser.contentWindow.getSelection()), "t", "First t should be selected.");

Ditto.

@@ +522,5 @@
>                      getService(Components.interfaces.nsIPrefBranch);
>        prefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 1);
>  
>        enterStringIntoFindField(SEARCH_TEXT.toUpperCase());
> +      is(String(gBrowser.contentWindow.getSelection()), "", "Not found.");

Ditto.

@@ +527,4 @@
>  
>        prefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 0);
>        setTimeout(function() {
> +        is(String(gBrowser.contentWindow.getSelection()), SEARCH_TEXT, "Search text should be selected.");

Ditto.

::: toolkit/content/tests/chrome/test_datepicker.xul
@@ +47,5 @@
>    testtag_datepicker(document.getElementById("datepicker"), "", "datepicker");
>    testtag_datepicker(dppopup, "popup", "datepicker popup");
>  
>    var gridpicker = document.getElementById("datepicker-grid");
> +  is(gridpicker.monthField.selectedIndex, "3", "datepicker grid correct month is initially selected");

It seems more than a bit strange for an index to be a string.  Is this definitely right, and not an underlying bug?  I tried tracking where this derives from by hand and got slightly lost.

::: toolkit/content/tests/chrome/test_progressmeter.xul
@@ +50,3 @@
>    n1.value = 150;
>    n1.max = 120;
> +  is(n1.value, "120", "max lowered below value");

Honestly it seems a bit strange for these getters to return strings, but they clearly do, consistently, so...

::: toolkit/content/tests/chrome/xul_selectcontrol.js
@@ +132,5 @@
>    test_nsIDOMXULSelectControlElement_States(element, testid + "removeItemAt", 1,
>          selectionRequired ? seconditem : null, selectionRequired ? 0 : -1,
>          selectionRequired ? secondvalue : "");
>  
> +  is(removeditem.control, undefined, testid + "control not set");

From all the examination of instances of "control" (with quotes) in toolkit/content/widgets, I can't find a reason for this to ever be the case (let alone *always* be the case).  Please file a followup and investigate the circumstances under which this happens.

(Note that because removeditem is firstitem from line 131, and firstitem had a control a ways up in this file, this does *not* appear to just be a totally non-existent property detection.  What exactly is wrong here, I wouldn't know without some debugging beyond visual inspection.)

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> Comment on attachment 8580260 [details] [diff] [review]
> Use === for SimpleTest.is
> 
> Review of attachment 8580260 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It goes without saying that you should feel free to parcel out these changes
> into separate revisions as far as you want, to lock in what's good while
> letting what's tricky move at its own pace.
> 
> ::: accessible/tests/mochitest/attributes.js
> @@ +142,5 @@
> >  
> >    var errorMsg = " for " + aID + " at offset " + aOffset;
> >  
> > +  is_loosely(startOffset.value, aStartOffset, "Wrong start offset" + errorMsg);
> > +  is_loosely(endOffset.value, aEndOffset, "Wrong end offset" + errorMsg);
> 
> I think you can use |is| here if you change this code in
> accessible/tests/mochitest/common.js:
> 
>     case "textAttrs": {
>       var prevOffset = -1;
>       for (var offset in accTree[prop]) {
>         if (prevOffset !=- 1) {
>           var attrs = accTree[prop][prevOffset];
>           testTextAttrs(acc, prevOffset, attrs, { }, prevOffset, offset,
> true);
>         }
>         prevOffset = offset;
>       }
> 
>       if (prevOffset != -1) {
>         var charCount = getAccessible(acc,
> [nsIAccessibleText]).characterCount;
>         var attrs = accTree[prop][prevOffset];
>         testTextAttrs(acc, prevOffset, attrs, { }, prevOffset, charCount,
> true);
>       }
> 
>       break;
>     }
> 
> by putting a + in front of |offset| in that first call to testTextAttrs.  If
> that doesn't work, please fix this to not is_loosely in a followup.

Done.

> ::: accessible/tests/mochitest/events.js
> @@ +1729,5 @@
> >      is(aEvent.start, this.startOffset,
> >         "Wrong start offset for " + prettyName(aID));
> >      is(aEvent.length, modifiedTextLen, "Wrong length for " + prettyName(aID));
> >      var changeInfo = (aIsInserted ? "inserted" : "removed");
> > +    is_loosely(aEvent.isInserted, aIsInserted,
> 
> I don't think this change is needed, so long as you change this code in
> accessible/tests/mochitest/events/test_text_alg.html from using 0/1 to using
> false/true:
> 
>     const kRemoval = 0;
>     const kInsertion = 1;

Done.

> @@ +1838,5 @@
> >  
> >      if (!event)
> >        return;
> >  
> > +    is_loosely(event.isExtraState, aIsExtraState,
> 
> I think you can leave this alone if you change
> accessible/tests/mochitest/states.js to have
> 
> const kOrdinalState = false;
> 
> instead of
> 
> const kOrdinalState = 0;

Done.

> ::: accessible/tests/mochitest/events/test_tree.xul
> @@ +261,5 @@
> >        [
> >          new nameChangeChecker("invalidateRow: ", 1, 0),
> >          new nameChangeChecker("invalidateRow: ", 1, 1),
> >          new rowNameChangeChecker("invalidateRow: ", 1),
> > +        new treeInvalidatedChecker("invalidateRow", 1, 1, null, undefined)
> 
> I don't think either of these changes are desirable.  The problem appears to
> be that inside treeInvalidatedChecker, we have this code:
> 
>         try {
>           var startCol = propBag.getPropertyAsInt32("startcolumn");
>         } catch (e if e.name == 'NS_ERROR_NOT_AVAILABLE') {
>           startCol = null;
>         }
>         is(startCol, aStartCol,
>            "Wrong 'startcolumn' of 'treeInvalidated' event on " + aMsg);
> 
> That code's all fine.  It defaults startCol to null if the
> getPropertyAsInt32 throws.  But look at what we have for aEndCol!
> 
>         try {
>           var endCol = propBag.getPropertyAsInt32("endcolumn");
>         } catch (e if e.name == 'NS_ERROR_NOT_AVAILABLE') {
>           startCol = null;
>         }
>         is(endCol, aEndCol,
>            "Wrong 'endcolumn' of 'treeInvalidated' event on " + aMsg);
> 
> The catch-code looks pretty clearly like it should be setting |endCol| --
> pure copypasta.
> 
> So change that instead of this.

Done

> ::: accessible/tests/mochitest/table/test_sels_listbox.xul
> @@ +81,1 @@
> >             aId + ": no columns should be selected");
> 
> So if you look at the callers of this method, *every* single one passes
> |aSelIdexesArray === null|.  So you can remove the if-test and else-block.
> 
> But...I think someone misunderstood how size_is in XPIDL is normally used. 
> The normal way to call a method taking an out length and out
> array/retval/size_is(out length) is to call the method but not pass an
> outparam for the array-out argument, then look at the array through the
> return value, like so.
>  
>       // getSelectedColumns
>       var selColsCount = {};
>       var colIndices = aAcc.getSelectedColumnIndices(selColsCount);
>       // colIndices is an array of column indexes, selColsCount.value is its
> length
> 
> In fact, per CallMethodHelper::GatherAndConvertResults() it looks like
> retval outparam arguments aren't even touched, to have a .value added to
> them.  So this code compares null/undefined against a non-existent property,
> which is rather pointless.  So really, you should convert this to:
> 
>       // getSelectedColumns
>       var selColsCount = {};
>       var colIndices = aAcc.getSelectedColumnIndices(selColsCount);
> 
>       is(selColsCount.value, aSelCount,
>          aId + ": wrong number of selected columns");
>       is(colIndices.length, aSelCount,
>          aId + ": wrong number of selected columns");
>
> @@ +120,1 @@
> >             aId + ": no row should be selected");
> 
> The same size_is XPIDL issues are present here, too.  Please convert this as
> follows:
> 
>       // getSelectedRows
>       var selRowsCount = {};
>       var rowIndices = aAcc.getSelectedRowIndices(selRowsCount);
> 
>       is(selRowsCount.value, aSelCount,
>          aId + ": wrong number of selected rows");
> 
>       if (!aSelIndexesArray) {
>         is(rowIndices, 0,
>            aId + ": no row should be selected");
>       } else {
>         for (var i = 0; i < rowIndices.length; i++) {
>           is(rowIndices[i], aSelIndexesArray[i],
>              aId + ": wrong selected row index " + i);
>         }
>       }
> 
> This of course assumes that when |aSelIndexesArray === null|, an empty array
> really truly is returned.  Try before you buy.
> 
> Also: before, this code happened not to implode because |i < selCols.length|
> was |0 < undefined| on first iteration, so no testing happened at all.  (!) 
> Extra reason to be careful making the change.
>
> @@ +158,5 @@
> >        is(selColsCount.value, aSelCount,
> >           aId + ": wrong number of selected cells");
> >  
> >        if (!aSelIndexesArray) {
> > +        is(selCols.value, undefined,
> 
> Same set of changes as for the row-code above needed, same reasons and all. 
> Just make sure to rename stuff away from row to cell (just as the current
> code refers to columns but really means cells).  :-\

Left this mess alone for now.

> ::: caps/tests/mochitest/test_disableScript.xul
> @@ +76,5 @@
> >      let deferred = Promise.defer();
> >      // We need to append 'name' to avoid running afoul of recursive frame detection.
> >      let frameURI = uri + "?name=" + name;
> >      navigateFrame(ifr, frameURI).then(function() {
> > +      is(String(ifr.contentWindow.location), frameURI, "Successful load");
> 
> I'd be inclined to do ifr.contentWindow.location.href instead.
> 
> ::: docshell/test/navigation/test_bug430723.html
> @@ +58,5 @@
> >    };
> >  }
> >  
> >  var step1 =function() {
> > +  window.is(String(testWindow.location), gTallRedBoxURI, "Ensure red page loaded.");
> 
> Hmm, there are probably a lot of comparisons of .location to a string. 
> s/\.location/\.location\.href/g then.

I'm not so inclined :)

> ::: dom/apps/tests/test_packaged_app_common.js
> @@ +171,2 @@
> >      }
> >      if (aExpectedApp.short_name) {
> 
> The function checkInstalledApp in
> dom/apps/tests/test_packaged_app_install.html appears to be unused.  Please
> remove it?

Done.

> @@ +175,5 @@
> >        }
> >        is(aApp.updateManifest.short_name, aExpectedApp.short_name, "Check short name mini-manifest");
> >      }
> >      if (aApp.manifest) {
> > +      is_loosely(aApp.manifest.version, aVersion, "Check version");
> 
> It looks to me like |aApp.manifest.version| for our packaged tests probably
> always a string, like "2" or "3.6" or "37.0.1" or whatever.  So |aVersion|
> should always be a string.  But many callers pass in whole numbers instead
> right now.  Please change them to strings in a followup.  This is the edited
> list of all of them, cut down to the places that need fixing, I think:
> 
> [jwalden@find-waldo-now src]$ ack checkAppState ../../dom/apps/tests/
> ../../dom/apps/tests/test_signed_pkg_install.html
> 137:        PackagedTestHelper.checkAppState(gApp, 1, expected,
> 236:        PackagedTestHelper.checkAppState(gApp, 1, expected,

04:23:14     INFO -  263 INFO TEST-UNEXPECTED-FAIL | dom/apps/tests/test_signed_pkg_install.html | Check version - got 1, expected "1"
04:23:15     INFO -  296 INFO TEST-UNEXPECTED-FAIL | dom/apps/tests/test_signed_pkg_install.html | Check version - got 1, expected "1"

> ../../dom/apps/tests/test_import_export.html
> 232:      PackagedTestHelper.checkAppState(PackagedTestHelper.gApp, 2,
> expected,
> 
> ../../dom/apps/tests/test_packaged_app_install.html
> 74:    PackagedTestHelper.checkAppState(evt.application, aVersion,
> aExpectedApp,
> 243:        PackagedTestHelper.checkAppState(PackagedTestHelper.gApp, 2,
> expected,
> 330:        PackagedTestHelper.checkAppState(PackagedTestHelper.gApp, 3,
> expected,
> 348:        PackagedTestHelper.checkAppState(PackagedTestHelper.gApp, 3,
> expected,
> 
> Also this:
> 
> ../../dom/apps/tests/test_packaged_app_update.html
> 73:  PackagedTestHelper.checkAppState(PackagedTestHelper.gApp,
> aExpectedVersion,
> 
> which corresponds to, I think:
> 
> 175:        checkLastAppState.bind(undefined, miniManifestURL, false, false,
> 176:                               2, PackagedTestHelper.next);
> 
> 235:    updateApp(true, 2, 3);
> 
> 254:    updateApp(true, 3, 4);
> 
> 261:    updateApp(false, 4, 4, true);
> 
> Basically I think all the literal numbers in the above are the exact things
> that need changing, to make them strings.

Done.

> ::: dom/base/test/test_XHRSendData.html
> @@ +231,5 @@
> >        is(xhr.getResponseHeader("Result-Content-Type"), null);
> >      }
> >  
> >      if (test.resContentLength) {
> > +      is(xhr.getResponseHeader("Result-Content-Length"), String(test.resContentLength), "Wrong Content-Length sent");
> 
> This is getting long enough to triple-line it, IMO.

Done.

> ::: dom/base/test/test_bug346485.html
> @@ +26,5 @@
> >   */
> >  
> >  var o = document.getElementById('o');
> >  
> > +is(String(o.htmlFor), 'a b',
> 
> Would have seemed preferable to me to have
> 
> function checkHtmlFor(htmlFor, arr)
> {
>   var len = htmlFor.length;
>   is(len, arr.length, htmlFor + ": incorrect htmlFor length");
>   for (var i = 0; i < len; i++)
>     is(htmlFor[i], arr[i], htmlFor + ": wrong element at " + i);
> }
> 
> but I guess this is what was happening before.  Maybe a followup tweak or
> something?  Or if these are definitely supposed to be testing
> stringification, then add .toString() to them.

Done.

> ::: dom/base/test/test_domrequesthelper.xul
> @@ +47,2 @@
> >        is(dummy._messages, undefined, "Messages is undefined");
> > +      is_loosely(dummy._window, undefined, "Window is undefined");
> 
> is(..., null, ...) for these

I tried; I believe the property doesn't always exist when coming through here. I'd rather not waste any more time on this test.

> I think you can remove the _messages test.  The only two hits for the string
> in this file are these two in this patch.  There are no hits for it in
> DOMRequestHelper.jsm.  It looks dead to me.  Get someone who's written this
> code to r= the reomval.

Scope creep.

> @@ +63,2 @@
> >        is(dummy._messages, undefined, "Messages is undefined");
> > +      is_loosely(dummy._window, undefined, "Window is undefined");
> 
> is(..., null, ...) for these

Ditto.

> @@ +387,5 @@
> >          var req_ = dummy.getRequest(id);
> >          is(req, req_, "Got correct request");
> >          dummy.removeRequest(id);
> >          req = dummy.getRequest(id);
> > +        is(req, undefined, "No request");
> 
> Any chance you could make a followup or something to make this check for
> null, after making DOMRequestHelper.jsm explicitly return null rather than
> fall off the end of its function?

I don't really care what DOMRequestHelper does.

> ::: dom/base/test/test_url.html
> @@ +310,5 @@
> >  
> >      url = new URL("http://localhost/");
> >      url.host = "[2001::1]:30";
> >      is(url.hostname, "[2001::1]", "IPv6 hostname");
> > +    is(url.port, "30", "Port");
> 
> ...URL.prototype.port returns a string, not a number?  :-\  That seems a bit
> strange to me.

It does.

> ::: dom/bindings/test/test_dom_xrays.html
> @@ +43,2 @@
> >        } else {
> > +        is_loosely(obj[name], value, "Should get the right value for \"" + name + "\" through Xrays");
> 
> File a bug to change this to is().  I can't think why loose comparisons have
> any value here, except as transition.

Bug 1153516.

> ::: dom/browser-element/mochitest/browserElementTestHelpers.js
> @@ +212,5 @@
> >          // otherwise we'll expect /every/ priority/LRU change to match
> >          // expectedPriority/expectedLRU.
> >          observed = true;
> >  
> > +        is(lru, String(expectedLRU),
> 
> Don't do this -- please change
> 
> dom/browser-element/mochitest/priority/test_ForegroundLRU.html
> 55:      expectPriorityWithLRUSet(childID, 'FOREGROUND', 1)
> 64:    var p = expectPriorityWithLRUSet(childID, 'FOREGROUND', 0)
> 
> to pass strings instead.

Done.

> ::: dom/browser-element/mochitest/browserElement_ReloadPostRequest.js
> @@ +99,5 @@
> >        is(e.detail.buttons[1].message, 'cancel');
> >        is(e.detail.message, expectedMessage.message);
> >        is(e.detail.buttons.length, 2);
> >        is(e.detail.showCheckbox, false);
> > +      is(e.detail.checkMessage, undefined);
> 
> That's a strange value for a property of an Event to have.  I looked a
> little deeper -- are we 100% certain this shouldn't be checkboxMessage?  Get
> a browser-element person to confirm (pun intended), but I very much suspect
> this is a bug in the test.

Done.

> ::: dom/contacts/tests/test_contacts_basics.html
> @@ +149,5 @@
> >        checkContacts(createResult1, properties1);
> >        // Some manual testing. Testint the testfunctions
> >        // tel: [{type: ["work"], value: "123456", carrier: "testCarrier"} , {type: ["home", "fax"], value: "[phone]"}],
> >        is(findResult1.tel[0].carrier, "testCarrier", "Same Carrier");
> > +      is(String(findResult1.tel[0].type), "work", "Same type");
> 
> Based on the comment above, can this be tel[0].type[0] instead of an
> explicit coercion?
> 
> @@ +157,5 @@
> >  
> >        is(findResult1.adr[0].countryName, "country 1", "Same country");
> >  
> >        // email: [{type: ["work"], value: "[e-mail]"}]
> > +      is(String(findResult1.email[0].type), "work", "Same Type");
> 
> .type[0]?
> 
> ::: dom/contacts/tests/test_contacts_basics2.html
> @@ +229,5 @@
> >      req.onsuccess = function () {
> >        ok(cloned.id, "The contact now has an ID.");
> >        is(cloned.email[0].value, "new email!", "Same Email");
> >        isnot(createResult1.email[0].value, cloned.email[0].value, "Clone has different email");
> > +      is(String(cloned.givenName), "Tom", "New Name");
> 
> Would [0] work here as well?

For all these, that would work, but then I'd kinda need to add a length check. I've left them like this for now.

> ::: dom/events/test/test_bug689564.html
> @@ +30,5 @@
> >     "div should have an onscroll handler");
> >  
> >  div.setAttribute("onpopstate", "div");
> >  is(window.onpopstate, null, "div should not forward onpopstate");
> > +is(div.onpopstate, undefined, "div should not have onpopstate handler");
> 
> is("onpopstate" in div, false, ...) would be a better test, if it works. 
> Please do that if it does.

Done.

> ::: dom/events/test/test_dragstart.html
> @@ +167,5 @@
> >    is(dt.types.length, 0, "invalid type setData");
> >    is(dt.getData(""), "", "invalid type getData"),
> >    dt.mozSetDataAt("", "Invalid Type", 0);
> >    is(dt.types.length, 0, "invalid type setDataAt");
> > +  is(dt.mozGetDataAt("", 0), undefined, "invalid type getDataAt"),
> 
> So I think this happens to be correct according to the code:
> 
> void
> DataTransfer::MozGetDataAt(JSContext* aCx, const nsAString& aFormat,
>                            uint32_t aIndex,
>                            JS::MutableHandle<JS::Value> aRetval,
>                            mozilla::ErrorResult& aRv)
> {
>   nsCOMPtr<nsIVariant> data;
>   aRv = MozGetDataAt(aFormat, aIndex, getter_AddRefs(data));
>   if (aRv.Failed()) {
>     return;
>   }
> 
>   if (!data) {
>     return; // we would end up here, not having set aRetval, leaving its
> prior value
>   }
> 
> But I'm not certain it's what was *supposed* to happen. 
> https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer#mozGetDataAt.
> 28.29 documents semantics as "Retrieves the data associated with the given
> format for an item at the specified index, or null if it does not exist." 
> Note how it says null should be returned here, right?
> 
> So this should have a followup to fix mozGetDataAt, or to fix the
> documentation, and perhaps to adjust this test further.

Looks like a regression from the handlification of WebIDL APIs that return any; filed bug 1153517.

> @@ +506,1 @@
> >         testid + " getDataAt " + expectedtypes[f]);
> 
> By my skimming expecteddata[f] is always truthy, so I don't understand that
> ternary.  And it's not obvious to me why a loose comparison is needed for
> left-hand-side purposes.  Please file a followup to make this more precise
> about whatever it's doing.

Filed bug 1153519.

> ::: dom/html/test/forms/test_input_number_mouse_events.html
> @@ +52,5 @@
> >    input.value = 0;
> >  
> >    // Test click on spin-up button:
> >    synthesizeMouse(input, SPIN_UP_X, SPIN_UP_Y, { type: "mousedown" });
> > +  is(input.value, "1", "Test step-up on mousedown on spin-up button");
> 
> By the right-hand rule, I hope?

Do you want me to do anything here?

> ::: dom/html/test/forms/test_output_element.html
> @@ +105,5 @@
> >  }
> >  
> >  function checkHtmlForIDLAttribute(element)
> >  {
> > +  is(String(element.htmlFor), 'a b',
> 
> Same checkHtmlFor comment as on the other test.

Meh. As it says, DOMSettableTokenList is tested elsewhere.

> ::: dom/html/test/forms/test_step_attribute.html
> @@ +446,4 @@
> >        checkValidity(input, true, apply);
> >  
> >        input.value = '1.5';
> > +      is(input.value, '2', "check that the value changes to the nearest valid step, choosing the higher step if both are equally close");
> 
> But but round to nearest even is the only rounding mode evar111!11!cos(0)!
> 
> ::: dom/html/test/forms/test_valueasdate_attribute.html
> @@ +187,5 @@
> >      element.value = data[0];
> > +    if (data[1]) {
> > +      is(String(element.valueAsDate), "Invalid Date",
> > +         "valueAsDate should return null "  +
> > +         "when the element value is not a valid date");
> 
> This message is wrong, something like s/return null/be an invalid Date
> object/.

Done.

> ::: dom/html/test/test_bug332893-3.html
> @@ +44,5 @@
> >                
> >  	var form1 = document.getElementById("form1");
> >  	var form2 = document.getElementById("form2");
> >  
> > +	is(form1.elements.length, 2, "Form 1 has the correct length");
> 
> wat @ someone comparing a length to the corresponding string in this file o_O

?

> ::: dom/html/test/test_documentAll.html
> @@ +30,5 @@
> >  p = document.getElementById("content");
> >  
> >  // Test that several elements with the same id or name behave correctly
> >  function testNumSame() {
> > +  is(document.all.id0, undefined, "no ids");
> 
> Ugh.  The previous code may well have been *assuming* that same-value
> semantics were in use for this testing function, because namedItem("id0")
> would return null here in marked contrast, despite getter behavior being
> declared to work through the namedItem method.  Progress, at least.
> 
> Could you add
> 
>   is(document.all.namedItem("id0"), null, "no ids");
> 
> next to this to highlight the behavioral difference, and to ensure it's
> explicitly tested in our tree?  (Probably is, but no sense taking chances.)
> 
> @@ +78,5 @@
> >  id3list = document.all.id3;
> >  rC(child[3]);
> >  is(id3list.length, 1, "now one length");
> >  rC(child[5]);
> > +is(document.all.id3, undefined, "now none");
> 
> Also a sibling namedItem test, please.
> 
> @@ +134,5 @@
> >      is(document.all[nameval], e, "should have name");
> >      hasName.shift();
> >    }
> >    else {
> > +    is(document.all[nameval], undefined, "shouldn't have name");
> 
> And again.

Done.

> ::: dom/indexedDB/test/unit/test_multientry.js
> @@ +89,5 @@
> >        is(req.result.primaryKey, test.indexes[j].k, "found expected index primary key at index " + j + testName);
> >        req.result.continue();
> >      }
> >      e = yield undefined;
> > +    ok(req.result == null, "exhausted indexes");
> 
> Why not is(req.result, null, ...)?
> 
> @@ +101,5 @@
> >        is(req.result.primaryKey, test.indexes[j].k, "found expected temp index primary key at index " + j + testName);
> >        req.result.continue();
> >      }
> >      e = yield undefined;
> > +    ok(req.result == null, "exhausted temp index");
> 
> And same here.
> 
> @@ +185,5 @@
> >        is(req.result.primaryKey, indexes[j].k, "found expected index primary key at index " + j + testName);
> >        req.result.continue();
> >      }
> >      e = yield undefined;
> > +    ok(req.result == null, "exhausted indexes");
> 
> And same.
> 
> @@ +197,5 @@
> >        is(req.result.primaryKey, indexes[j].k, "found expected temp index primary key at index " + j + testName);
> >        req.result.continue();
> >      }
> >      e = yield undefined;
> > +    ok(req.result == null, "exhausted temp index");
> 
> Same.

Sometimes they're undefined, and is_loosely doesn't work in xpcshell (*sigh*).

> ::: dom/indexedDB/test/unit/test_setVersion_abort.js
> @@ +45,5 @@
> >  
> >    event = yield undefined;
> >    is(event.type, "abort", "Got transaction abort event");
> >    is(event.target, transaction, "Right target");
> > +  is(event.target.transaction, undefined, "No transaction");
> 
> I'm not sure I believe this.  Why would a transaction have a .transaction
> property?  khuey doesn't believe it either -- and he said just remove this
> line, which was what I was thinking, so do that.
> 
> ..and beyond that, several lines further down, is code similar to this where
> event.target is a request, suggesting this is bad copypasta.  So yeah. 
> Remove this line.

Done.

> ::: dom/indexedDB/test/unit/test_setVersion_events.js
> @@ +39,1 @@
> >      is(event.target, db1, "Correct target");
> 
> So if |event.target| is an IDBDatabase, and IDBDatabase has no source
> property, and it inherits from EventTarget only (which just adds in
> add/removeEventListener and dispatchEvent)...why should there be a .source
> property here?  My guess is this is just comparing a non-existent property,
> i.e. undefined, against null.
> 
> So this test probably should be removed.  Same for the other two changed
> lines in this file.  Confirm with khuey or someone who knows indexeddb
> before going ahead with this, as usual.

Replaced by is("source" in event.target, false), except for the case where the target is an IDBRequest.

> ::: dom/media/test/test_audio2.html
> @@ +20,5 @@
> >      return;
> >    manager.started(token);
> >    var a1 = new Audio(test.name);
> >    is(a1.getAttribute("preload"), "auto", "Preload automatically set to auto");
> > +  is(String(a1.src.match("/[^/]+$")), "/" + test.name, "src OK");
> 
> This is better written
> 
> ok(a1.src.endsWith("/" + test.name), "src OK");

You and your newfangled JS. Done.

> ::: dom/plugins/test/mochitest/test_npruntime_identifiers.html
> @@ +33,5 @@
> >        is(reflector[prop], prop, "Property " + prop);
> >      }
> >  
> > +    for (i = -10; i < 0; ++i) {
> > +      is(reflector[i], String(i), "Property " + i);
> 
> Dollars to donuts says this test, had it been a string-versus-string test
> all along, would have been broken awhile back, before jsid of -1 changed
> from being an integer-valued jsid to being a string-valued jsid.  All the
> better for us, I guess.
> 
> ::: dom/plugins/test/mochitest/test_propertyAndMethod.html
> @@ +33,5 @@
> >          delete pluginProto.propertyAndMethod;
> >          ok(isNaN(plugin.propertyAndMethod + 0), "Shouldn't be set yet!");
> >  
> >          plugin.propertyAndMethod = 5;
> > +        is(+plugin.propertyAndMethod, 5, "Should be set to 5!");
> 
> I confess to much confusion as to why this is necessary.  The test plugin
> does INT32_TO_NPVARIANT(5, *result), which I would really really really have
> thought would caused the getter to return the number value 5.  I understand
> that a non-primitive value would be wrapped in an NPObject and such and be
> weird, but I didn't think that applied to primitives.
> 
> Please rope in some plugin person to edumacate me here.

johns? I already wasted too much time trying to figure out this crap.

> ::: dom/tests/mochitest/webapps/test_install_app.xul
> @@ +54,5 @@
> >      ok(app.installTime + fuzzySpan >= beforehand, "install time is after install() call");
> >      ok(app.installTime <= Date.now() + fuzzySpan, "install time is before install success");
> >      is(app.manifestURL, url, "manifest URL");
> >      is(app.manifest.name, "Basic App", "manifest.name");
> > +    is(String(app.manifest.installs_allowed_from), "*",
> 
> Split this into
> 
>     is(app.manifest.installs_allowed_from.length, 1, "allowed-from length
> must be 1");
>     is(app.manifest.installs_allowed_from[0], "*", "allowed from anywhere");
> 
> rather than invoke stringification code.

Done.

> ::: dom/tests/mochitest/webapps/test_launch_paths.xul
> @@ +54,5 @@
> >      ok(app.installTime + fuzzySpan >= beforehand, "install time is after install() call");
> >      ok(app.installTime <= Date.now() + fuzzySpan, "install time is before install success");
> >      is(app.manifestURL, url, "manifest URL");
> >      is(app.manifest.name, "Basic App with Launch Paths", "manifest.name");
> > +    is(String(app.manifest.installs_allowed_from), "*",
> 
> is(app.manifest.installs_allowed_from.length, 1, ...);
> is(app.manifest.installs_allowed_from[0], "*");
> 
> as requested elsewhere.

Done.

> ::: editor/libeditor/tests/spellcheck.js
> @@ +9,5 @@
> >      return false;
> >    }
> >  
> >    for (var i = 0; i < numWords; ++i) {
> > +    var word = String(sel.getRangeAt(i));
> 
> Preference for explicit .toString() instead of hazier conversion semantics.
> 
> ::: editor/libeditor/tests/test_bug1100966.html
> @@ +38,5 @@
> >  
> >        var sel = getSpellCheckSelection();
> >        is(sel.rangeCount, 2, "We should have two misspelled words");
> > +      is(String(sel.getRangeAt(0)), "fivee", "Correct misspelled word");
> > +      is(String(sel.getRangeAt(1)), "sixx", "Correct misspelled word");
> 
> Same .toString() preference.

I prefer String(), if you don't mind.

> ::: editor/libeditor/tests/test_bug596333.html
> @@ +79,5 @@
> >      append(" workd");
> >      edit.blur();
> >      onSpellCheck(edit, function () {
> >        gMisspeltWords.push("workd");
> > +      ok(isSpellingCheckOk(getEditor(), gMisspeltWords),
> 
> Why are you passing any arguments to isSpellingCheckOk?  It doesn't take
> any.  Maybe it should, and not use global mutable state, but that should be
> separately changed, and not pointlessly halfway like here.

This belonged in the other patch in this bug. Fixed.

> ::: js/xpconnect/tests/chrome/test_bug571849.xul
> @@ +30,1 @@
> >      ok(/Text/.test(output[1]), "the 0th element was a text node");
> 
> Instead of
> 
>     var output;
> 
> let's instead do
> 
>     var index, value;
>     for (let i in Iterator(docnodes))
>         index = i[0], value = i[1];
>     is(index, 0, ...);
>     ok(/Text/.test(value), ...);
> 
> for more readability.

Done.

> ::: js/xpconnect/tests/mochitest/test_bug691059.html
> @@ +41,1 @@
> >         "prototype.onmouseleave should be settable, but the value should stay null");
> 
> Uh, these are just bizarre.  Of *course* setting
> Document.prototype.onmouseenter has no effect on Document.onmouseenter. 
> They're totally different things.
> 
> Two things to change here.  First, use
> 
>   is("onmouseenter" in obj, false, ...);
> 
> to capture the real test, which is that there's no property (not allowing a
> property with value undefined, or an accessor returning such, to be a
> head-fake).  Second, use something like
> 
> "setting <Interface>.prototype.onmouseenter has no effect on the
> non-existent <Interface>.onmouseenter"
> 
> instead of the current message that would blatantly contradict the test as
> you've written it.

Done.

> ::: layout/style/test/test_flexbox_flex_grow_and_shrink.html
> @@ +77,2 @@
> >  advance_clock(1000);
> > +is(+cs.flexGrow, 10, "flexGrowTwoToThree at 1.5s");
> 
> For all the is(...) tests you're changing, don't coerce the computed-style
> property -- compare against a string instead.  Adding in an extra coercion
> means more code to execute to make the comparison, that might possibly be
> broken at some time or other in ways that make the test start spuriously
> succeeding or so.  In contrast is(cs.flexGrow, "10", ...) is not vulnerable
> to such fallout.
> 
> The is_approx tests do make sense to do as you've done them, tho.

Done.

> ::: testing/mochitest/tests/SimpleTest/SimpleTest.js
> @@ +291,5 @@
> >      SimpleTest.ok(pass, name, diag);
> >  };
> >  
> >  /**
> > + * Roughly equivalent to ok(a===, name)
> 
> ok(a == b, name)

Done.

> ::: toolkit/components/microformats/tests/test_Microformats.html
> @@ +252,5 @@
> >    origdate = Microformats.iso8601FromDate(jsdate, true);
> >    is(dateCal.dtstart, origdate, "date round trip");
> >  
> >    var dateCal = new hCalendar(document.getElementById("vcal_vcard"));
> > +  is(String(dateCal.description), "Mozilla's Birthday", "vcard in vcal");
> 
> Add the same sort of isOneElementArrayContaining helper and use that in all
> this file's changes.
> 
> ::: toolkit/components/microformats/tests/test_Microformats_hCalendar.html
> @@ +229,5 @@
> >    is(hcalendar.summary, "Bad Movie Night - Gigli (blame mike spiegelman)", "06-component-vevent-uri-relative - summary");
> >    is(hcalendar.dtstart, "2006-01-15T00:00:00", "06-component-vevent-uri-relative - dtstart");
> >  
> >    hcalendar = new hCalendar(document.getElementById("07-component-vevent-description-simple"));
> > +  is(String(hcalendar.description), "Project xyz Review Meeting Minutes", "07-component-vevent-description-simple - description");
> 
> Add a helper
> 
> function isOneElementArrayContaining(arr, contents, msg)
> {
>   is(arr.length, 1, msg + ": length must be 1");
>   is(arr[0], contents, msg + ": must contain expected value");
> }
> 
> Then use that all three locations in this test.  Please have a microformats
> person review this, tho -- I couldn't be 100% certain from specs that the
> description item was supposed to be a list.
>
> ::: toolkit/components/microformats/tests/test_Microformats_hCard.html
> @@ +575,5 @@
> >  
> >    hcard = new hCard(document.getElementById("01-tantek-basic"));
> >  
> >    is(hcard.fn, "Tantek Çelik", "01-tantek-basic - fn");
> > +  is(String(hcard.url), "http://tantek.com/", "01-tantek-basic - url");
> 
> ...okay, this file *really* wants that helper function, rather than casting
> arrays to strings a hundred times or so.

I'd really rather leave these alone.

> :::
> toolkit/components/url-classifier/tests/mochitest/classifiedAnnotatedFrame.
> html
> @@ +59,5 @@
> >        nodeMatch |=
> >          (blockedTrackingNodes[i] == document.getElementById(badids[j]));
> >      }
> >  
> >      allNodeMatch &= nodeMatch;
> 
> Could you change this to allNodeMatch = allNodeMatch && nodeMatch?  And same
> in the loop below.

Done.

> ::: toolkit/content/tests/chrome/findbar_window.xul
> @@ +450,2 @@
> >            "Currently highlighted match should be at " + aTest.current);
> > +        is(aMatches[2], String(aTest.total),
> 
> Mild preferences for using .toString() to convert numbers to strings, than
> to use String().  (Honestly I guess this applied to the entire patch, I just
> didn't recognize it early enough.  Don't feel pressured to go back and fix
> the entire patch, that's too much.  :-) )
> 
> @@ +501,5 @@
> >                        .getService(Ci.nsIPrefBranch);
> >        prefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 0);
> >  
> >        enterStringIntoFindField("t");
> > +      is(String(gBrowser.contentWindow.getSelection()), "T", "First T should be selected.");
> 
> .toString() to invoke selection-specific conversion code, please.
> 
> @@ +506,4 @@
> >  
> >        prefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 1);
> >        setTimeout(function() {
> > +        is(String(gBrowser.contentWindow.getSelection()), "t", "First t should be selected.");
> 
> Ditto.
> 
> @@ +522,5 @@
> >                      getService(Components.interfaces.nsIPrefBranch);
> >        prefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 1);
> >  
> >        enterStringIntoFindField(SEARCH_TEXT.toUpperCase());
> > +      is(String(gBrowser.contentWindow.getSelection()), "", "Not found.");
> 
> Ditto.
> 
> @@ +527,4 @@
> >  
> >        prefsvc.setIntPref("accessibility.typeaheadfind.casesensitive", 0);
> >        setTimeout(function() {
> > +        is(String(gBrowser.contentWindow.getSelection()), SEARCH_TEXT, "Search text should be selected.");
> 
> Ditto.

I'd rather not go back and change all those.

> ::: toolkit/content/tests/chrome/test_datepicker.xul
> @@ +47,5 @@
> >    testtag_datepicker(document.getElementById("datepicker"), "", "datepicker");
> >    testtag_datepicker(dppopup, "popup", "datepicker popup");
> >  
> >    var gridpicker = document.getElementById("datepicker-grid");
> > +  is(gridpicker.monthField.selectedIndex, "3", "datepicker grid correct month is initially selected");
> 
> It seems more than a bit strange for an index to be a string.  Is this
> definitely right, and not an underlying bug?  I tried tracking where this
> derives from by hand and got slightly lost.

Might well be a bug. This is the current behaviour, though.

> ::: toolkit/content/tests/chrome/test_progressmeter.xul
> @@ +50,3 @@
> >    n1.value = 150;
> >    n1.max = 120;
> > +  is(n1.value, "120", "max lowered below value");
> 
> Honestly it seems a bit strange for these getters to return strings, but
> they clearly do, consistently, so...
> 
> ::: toolkit/content/tests/chrome/xul_selectcontrol.js
> @@ +132,5 @@
> >    test_nsIDOMXULSelectControlElement_States(element, testid + "removeItemAt", 1,
> >          selectionRequired ? seconditem : null, selectionRequired ? 0 : -1,
> >          selectionRequired ? secondvalue : "");
> >  
> > +  is(removeditem.control, undefined, testid + "control not set");
> 
> From all the examination of instances of "control" (with quotes) in
> toolkit/content/widgets, I can't find a reason for this to ever be the case
> (let alone *always* be the case).  Please file a followup and investigate
> the circumstances under which this happens.
> 
> (Note that because removeditem is firstitem from line 131, and firstitem had
> a control a ways up in this file, this does *not* appear to just be a
> totally non-existent property detection.  What exactly is wrong here, I
> wouldn't know without some debugging beyond visual inspection.)

You want to file this one?

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Some missed fixes

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Review fixes

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 8591379 [details] [diff] [review]
Review fixes

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

(In reply to :Ms2ger from comment #10)
> > ::: accessible/tests/mochitest/table/test_sels_listbox.xul
> Left this mess alone for now.

Fair, but please file the bug.

> I tried; I believe the property doesn't always exist when coming through
> here. I'd rather not waste any more time on this test.

Okay.  But we need to remove is_loosely eventually, because otherwise people will use it and then we'll be creeping back toward the current bad state.  File the bug, at least.

> > I think you can remove the _messages test.  The only two hits for the string
> > in this file are these two in this patch.  There are no hits for it in
> > DOMRequestHelper.jsm.  It looks dead to me.  Get someone who's written this
> > code to r= the reomval.
> 
> Scope creep.
> Ditto.

Fine.  But file a bug on it.

> > Any chance you could make a followup or something to make this check for
> > null, after making DOMRequestHelper.jsm explicitly return null rather than
> > fall off the end of its function?
> 
> I don't really care what DOMRequestHelper does.

Understandable.  But file the bug to make those changes.  If we're going to have an API, it needs to behave sanely, regardless whether you care or not.

> > ::: dom/html/test/forms/test_input_number_mouse_events.html
> > @@ +52,5 @@
> > >    input.value = 0;
> > >  
> > >    // Test click on spin-up button:
> > >    synthesizeMouse(input, SPIN_UP_X, SPIN_UP_Y, { type: "mousedown" });
> > > +  is(input.value, "1", "Test step-up on mousedown on spin-up button");
> > 
> > By the right-hand rule, I hope?
> 
> Do you want me to do anything here?

Just a physics joke.  http://hyperphysics.phy-astr.gsu.edu/hbase/tord.html

> > ::: dom/html/test/test_bug332893-3.html
> > @@ +44,5 @@
> > >                
> > >  	var form1 = document.getElementById("form1");
> > >  	var form2 = document.getElementById("form2");
> > >  
> > > +	is(form1.elements.length, 2, "Form 1 has the correct length");
> > 
> > wat @ someone comparing a length to the corresponding string in this file o_O
> 
> ?

Sorry, that was a comment on someone having previously done 

  is(form1.elements.length, "2", "Form 1 has the correct length");

comparing a numeric length with a string number.  Comparing a string against an expected number I can see, because you might not realize a property had a string value.  But lengths being intuitively numeric would have had me thinking no one would compare against a constant string, because that wouldn't be intuitive.

> > ::: dom/plugins/test/mochitest/test_propertyAndMethod.html
> > @@ +33,5 @@
> > >          delete pluginProto.propertyAndMethod;
> > >          ok(isNaN(plugin.propertyAndMethod + 0), "Shouldn't be set yet!");
> > >  
> > >          plugin.propertyAndMethod = 5;
> > > +        is(+plugin.propertyAndMethod, 5, "Should be set to 5!");
> > 
> > I confess to much confusion as to why this is necessary.  The test plugin
> > does INT32_TO_NPVARIANT(5, *result), which I would really really really have
> > thought would caused the getter to return the number value 5.  I understand
> > that a non-primitive value would be wrapped in an NPObject and such and be
> > weird, but I didn't think that applied to primitives.
> > 
> > Please rope in some plugin person to edumacate me here.
> 
> johns? I already wasted too much time trying to figure out this crap.

Yeah, rope in *someone* plugsiny.  Although, johns moved elsewhere a few months ago, so ideally not him.

> > Same .toString() preference.
> 
> I prefer String(), if you don't mind.

Not sure why, but okay.  Seems to me that String invokes strictly more code, which makes it harder to understand.

> > ::: toolkit/components/microformats/tests/test_Microformats.html
> > ::: toolkit/components/microformats/tests/test_Microformats_hCalendar.html
> > Add a helper
> > 
> > function isOneElementArrayContaining(arr, contents, msg)
> 
> I'd really rather leave these alone.

Please file a followup, at least.  Lax checking is bad whether it's via bad equality operations or by imprecise tests.

> > ::: toolkit/content/tests/chrome/test_datepicker.xul
> > @@ +47,5 @@
> > >    testtag_datepicker(document.getElementById("datepicker"), "", "datepicker");
> > >    testtag_datepicker(dppopup, "popup", "datepicker popup");
> > >  
> > >    var gridpicker = document.getElementById("datepicker-grid");
> > > +  is(gridpicker.monthField.selectedIndex, "3", "datepicker grid correct month is initially selected");
> > 
> > It seems more than a bit strange for an index to be a string.  Is this
> > definitely right, and not an underlying bug?  I tried tracking where this
> > derives from by hand and got slightly lost.
> 
> Might well be a bug. This is the current behaviour, though.

Maybe poke NeilAway to look at this?  He probably knows this XUL/widgets behavior off the top of his head well enough to talk about it if needed for a followup.

> > ::: toolkit/content/tests/chrome/xul_selectcontrol.js
> > @@ +132,5 @@
> > >    test_nsIDOMXULSelectControlElement_States(element, testid + "removeItemAt", 1,
> > >          selectionRequired ? seconditem : null, selectionRequired ? 0 : -1,
> > >          selectionRequired ? secondvalue : "");
> > >  
> > > +  is(removeditem.control, undefined, testid + "control not set");
> > 
> > From all the examination of instances of "control" (with quotes) in
> > toolkit/content/widgets, I can't find a reason for this to ever be the case
> > (let alone *always* be the case).  Please file a followup and investigate
> > the circumstances under which this happens.
> > 
> > (Note that because removeditem is firstitem from line 131, and firstitem had
> > a control a ways up in this file, this does *not* appear to just be a
> > totally non-existent property detection.  What exactly is wrong here, I
> > wouldn't know without some debugging beyond visual inspection.)
> 
> You want to file this one?

You're the one fixing this bug, you get to file.  :-)  Just copy-pasting this text into the bug report is good enough, this doesn't have to take any time to do.

::: js/xpconnect/tests/chrome/test_bug571849.xul
@@ +27,5 @@
> +    for (let i in Iterator(docnodes)) {
> +      index = i[0], value = i[1];
> +    }
> +    is(index, 0, "enumerated the 0th element");
> +    ok(/Text/.test(value), "the 0th element was a text node");

Hmm, we could have changed this to instanceof TextNode, I guess.  I assume you'll tryserver, worth throwing this in and seeing if it flies.  If not, no worries about sticking to what's there.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> ::: dom/plugins/test/mochitest/test_propertyAndMethod.html
> @@ +33,5 @@
> >          delete pluginProto.propertyAndMethod;
> >          ok(isNaN(plugin.propertyAndMethod + 0), "Shouldn't be set yet!");
> >  
> >          plugin.propertyAndMethod = 5;
> > +        is(+plugin.propertyAndMethod, 5, "Should be set to 5!");
> 
> I confess to much confusion as to why this is necessary.  The test plugin
> does INT32_TO_NPVARIANT(5, *result), which I would really really really have
> thought would caused the getter to return the number value 5.  I understand
> that a non-primitive value would be wrapped in an NPObject and such and be
> weird, but I didn't think that applied to primitives.
> 
> Please rope in some plugin person to edumacate me here.

Comment 16

[[e-mail]]

(In reply to Jeff Walden from comment #13)
> (From update of attachment 8591379 [details] [diff] [review])
> (In reply to Ms2ger from comment #10)
> > > > +  is(gridpicker.monthField.selectedIndex, "3", "datepicker grid correct month is initially selected");
> > > 
> > > It seems more than a bit strange for an index to be a string.  Is this
> > > definitely right, and not an underlying bug?  I tried tracking where this
> > > derives from by hand and got slightly lost.
> > 
> > Might well be a bug. This is the current behaviour, though.
> 
> Maybe poke NeilAway to look at this?  He probably knows this XUL/widgets
> behavior off the top of his head well enough to talk about it if needed for
> a followup.

The <gridpicker>'s month field is actually a <deck>, and the selected index was originally implemented as just an attribute on the deck. Bug 377696 then made it so that if the attribute is missing or empty then '0' is returned.

There seem to be very few consumers of <deck>'s selectedIndex getter in the tree, and even fewer who care about its type (I found two places comm-central where it gets converted to a number, one with a helpful comment, and also some possibly broken code in the old Venkman extension that wasn't sure whether it was a number).

To add to the confusion, <tabpanels> overrides <deck>'s selectedIndex property and returns a number (or -1 if the attribute is missing or empty, but it doesn't allow the selectedIndex to be set to -1, and the first panel probably gets displayed anyway).

Bug 362321 comment #2 points out that MDN documents it as being an integer.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: New test

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1615a80a965
https://hg.mozilla.org/integration/mozilla-inbound/rev/763263ec2efa
https://hg.mozilla.org/integration/mozilla-inbound/rev/f302ad6c981d

Comment 19

[(no longer active)]

Thanks Ms2ger!  Please write about this to dev.platform.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1615a80a965
https://hg.mozilla.org/mozilla-central/rev/763263ec2efa
https://hg.mozilla.org/mozilla-central/rev/f302ad6c981d

Comment 21

[Jim Mathies [:jimm]]

(In reply to :Ms2ger from comment #15)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> > ::: dom/plugins/test/mochitest/test_propertyAndMethod.html
> > @@ +33,5 @@
> > >          delete pluginProto.propertyAndMethod;
> > >          ok(isNaN(plugin.propertyAndMethod + 0), "Shouldn't be set yet!");
> > >  
> > >          plugin.propertyAndMethod = 5;
> > > +        is(+plugin.propertyAndMethod, 5, "Should be set to 5!");
> > 
> > I confess to much confusion as to why this is necessary.  The test plugin
> > does INT32_TO_NPVARIANT(5, *result), which I would really really really have
> > thought would caused the getter to return the number value 5.  I understand
> > that a non-primitive value would be wrapped in an NPObject and such and be
> > weird, but I didn't think that applied to primitives.
> > 
> > Please rope in some plugin person to edumacate me here.

Please file a follow up for investigation. I don't know the answer here.