Should check for error code from h3Line call

[dfellis]

Hey @nrabinowitz

So h3-node currently (ab)uses h3-js for unit testing by just confirming that the outputs of both are identical given semi-random inputs.

I've noticed intermittent failures in the h3Line test, and after digging through the code, I think the issue might be on the h3-js side.

We can see in the declaration of the h3Line function that it returns an integer value.

/** @brief Line of h3 indexes connecting two indexes */
int H3_EXPORT(h3Line)(H3Index start, H3Index end, H3Index *out);

And in the documentation on the implementation that the return value is an error/success status code

 * @return 0 on success, or another value on failure.

So the h3-node implementation of binding h3Line includes a check for that status

  int errorCode = h3Line(origin, destination, line);
  if (errorCode != 0) {
    napi_throw_error(env, "EINVAL", "Line cannot be calculated");
    free(line);
    return NULL;
  }

But the h3-js implementation of binding h3Line ignores that status

function h3Line(origin, destination) {
    const [oLower, oUpper] = h3IndexToSplitLong(origin);
    const [dLower, dUpper] = h3IndexToSplitLong(destination);
    const count = H3.h3LineSize(oLower, oUpper, dLower, dUpper);
    if (count < 0) {
        // We can't get the specific error code here - may be any of
        // the errors possible in experimentalH3ToLocalIj
        throw new Error('Line cannot be calculated');
    }
    const hexagons = C._calloc(count, SZ_H3INDEX);
    H3.h3Line(oLower, oUpper, dLower, dUpper, hexagons); // << Not reading the return value
    const out = readArrayOfHexagons(hexagons, count);
    C._free(hexagons);
    return out;
}

I'd normally just make a PR to fix this, but since you implemented the C version of the code and the JS bindings, I wanted to bring this up before, in case this was actually intentional?

[nrabinowitz]

I had a related conversation with @isaacbrodsky here: https://github.com/uber/h3-java/pull/36

You are correct, I should check the output of h3Line for completeness (I'm updating the title of this issue to reflect that). But with the current implementation of h3Line, it is (almost certainly) impossible to get a non-zero exit code here IFF you got a a value > 0 from h3LineSize. Both functions just delegate immediately to h3Distance under the hood.

So while you're right that we should check, the error condition here is (almost certainly) unreachable, and is unlikely to be the source of your bug. I think it's more likely you're running into undefined output due to whatever is behind https://github.com/uber/h3/issues/184.

[dfellis]

Hmm... So it looks like Isaac is planning to dig into that issue, but do you have any proposal right now? Maybe I should just confirm that 90+% of the runs succeed? (That would still have very intermittent failures if the random number generator ever does more than 10% lines through pentagons.)

[nrabinowitz]

Not sure. Possibilities here:

• Keep your current test with a threshold, as you note, and try to drop it later
• Implement separate property-based tests, as we do here: https://github.com/uber/h3-js/blob/master/test/h3core.spec.js#L1201 - these should work regardless of h3-js issues (though I think the failure cases might fail here too, if they're in the underlying lib)

I don't necessarily think that asserting identical output here is a big issue, since we explicitly don't guarantee stable output across versions - there are multiple "correct" answers here, so the important concern is guaranteeing the properties of the output, not necessarily the consistency of the output.

[dfellis]

Well, I am trying to guarantee that the major and minor version numbers for h3-node match those of h3-js so it can be a drop-in replacement (assuming no error condition reached) for anyone wanting a perf boost on Node apps.

Let me think about it a bit more. The random-but-valid input approach was nice as a kind of fuzzing, as well (which is how I found this discrepancy in the first place).

[nrabinowitz]

This issue should be covered by the new error handling in #139