test: use a valid comparefn for sort

[kfarnung]

According the the ES spec a valid comparefn returns a negative number,
zero, or a positive number. The behavior of returning a boolean is also
not consistent among JS engines.

For example:

$ eshost -ise "Object.keys({ a200: 4, a100: 1, a102: 3, a101: 2 }).sort(function (a, b) { return a < b; })"
## Source
print(Object.keys({ a200: 4, a100: 1, a102: 3, a101: 2 }).sort(function (a, b) { return a < b; }))

#### Chakra, V8, V8 --harmony
a200,a100,a102,a101

#### JavaScriptCore, SpiderMonkey, XS
a200,a102,a101,a100

Interestingly D8 and node-v8 disagree (EDIT: this is because of the Timsort introduction in V8 7.0):

$ ~/.jsvu/v8 -e "console.log(Object.keys({ a200: 4, a100: 1, a102: 3, a101: 2 }).sort(function (a, b) { return a < b; }))"
a200,a100,a102,a101
$ node -e "console.log(Object.keys({ a200: 4, a100: 1, a102: 3, a101: 2 }).sort(function (a, b) { return a < b; }))"
[ 'a200', 'a102', 'a101', 'a100' ]

With the updated compare function everyone agrees:

$ eshost -ise "Object.keys({ a200: 4, a100: 1, a102: 3, a101: 2 }).sort(function (a, b) { return a === b ? 0 : (a < b ? 1 : -1); })"
## Source
print(Object.keys({ a200: 4, a100: 1, a102: 3, a101: 2 }).sort(function (a, b) { return a === b ? 0 : (a < b ? 1 : -1); }))

#### Chakra, JavaScriptCore, SpiderMonkey, V8, V8 --harmony, XS
a200,a102,a101,a100

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@kfarnung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/958/pipeline

[richardlau]

#22992 has an alternative change. We need to decide between them.

[kfarnung]

Thanks @richardlau, that explains my test results. The D8 version I tested with was from the 7.x series.

I gave my opinion in the other PR, but to re-state it here: I don't see any other usages of localeCompare in the test code, so I'm not sure that we want to introduce one now.

[kfarnung]

Kicked off a CI: https://ci.nodejs.org/job/node-test-pull-request/17355/

[jackhorton]

Quick microbenchmark - the explicit compare seems much quicker across engines, fwiw. Chakra makes an attempt to cache localeCompare results (we took a known speed regression in RS5 with the switch to ICU, which is why the JSVU version of chakra is almost equal):

[F:\chakra\full x64debug] F:\chakra\full>eshost -i F:\Scratch\string-compare.js
## Source
function compare(a, b) {
    return a > b ? 1 : a < b ? -1 : 0;
}

function localeCompare(a, b) {
    return a.localeCompare(b);
}

function testCompare() {
    const start = Date.now();
    for (let i = 0; i < 10000; i++) {
        const result = compare("a", "b");
        if (result !== -1) {
            print("failed");
            return;
        }
    }
    print(`compare took ${Date.now() - start} seconds`);
}

function testLocaleCompare() {
    const start = Date.now();
    for (let i = 0; i < 10000; i++) {
        const result = localeCompare("a", "b");
        if (result !== -1) {
            print("failed");
            return;
        }
    }
    print(`localeCompare took ${Date.now() - start} seconds`);
}

testCompare();
testLocaleCompare();


#### V8
compare took 4 seconds
localeCompare took 26 seconds

#### SpiderMonkey
compare took 4 seconds
localeCompare took 19 seconds

#### Chakra (JSVU)
compare took 3 seconds
localeCompare took 5 seconds

#### Chakra (RS5/ICU 61)
compare took 3 seconds
localeCompare took 15 seconds

[kfarnung]

I don't think the perf difference is too important for this scenario, so I'll close my PR in favor of #22992. That one is a lot easier to read as an example.