SqliteErrors have wrong constructor in Jest environment

[wchargin]

An error thrown from Statement#run can, under some invocations, not
have constructor SqliteError. The behavior is as expected under Node,
but fails when run in a Jest test case.

Here is a repro with two files (package.json and test.js):

package.json

{
  "dependencies": {
    "better-sqlite3": "4.1.4",
    "jest": "23.5.0"
  }
}

test.js

#!/usr/bin/env node
const Database = require("better-sqlite3");

const db = new Database("/tmp/foo", { memory: true });

db.prepare("CREATE TABLE tab (col PRIMARY KEY)").run();
db.prepare("INSERT INTO tab (col) VALUES (1)").run();

let e;
try {
  db.prepare("INSERT INTO tab (col) VALUES (1)").run();
} catch (e_) {
  e = e_;
}
if (e == null) {
  throw new Error("Should have thrown (SQLITE_CONSTRAINT_PRIMARYKEY)");
}

const errorType = e.constructor;
const SqliteError = Database.SqliteError;
console.warn(errorType, SqliteError, errorType === SqliteError);

if (typeof it !== "undefined") {
  // Jest
  it("SqliteErrors should be SqliteErrors", () => {
    expect(errorType).toBe(SqliteError);
  });
} else {
  // Node
  console.log("1..1");
  console.log(errorType === SqliteError ? "ok 1" : "not ok 1");
}

When run with node ./test.js, this prints:

[Function: SqliteError] [Function: SqliteError] true
1..1
ok 1

When run with node ./node_modules/.bin/jest ./test.js, this prints:

FAIL ./test.js
  ✕ SqliteErrors should be SqliteErrors (12ms)

  ● SqliteErrors should be SqliteErrors

    expect(received).toBe(expected) // Object.is equality

    Expected: [Function SqliteError]
    Received: [Function SqliteError]

    Difference:

    Compared values have no visual difference.

      24 |   // Jest
      25 |   it("SqliteErrors should be SqliteErrors", () => {
    > 26 |     expect(errorType).toBe(SqliteError);
         |                       ^
      27 |   });
      28 | } else {
      29 |   // Node

      at Object.toBe (test.js:26:23)

  console.warn test.js:21
    [Function: SqliteError] [Function: SqliteError] false

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        1.047s
Ran all test suites matching /.\/test.js/i.

Using require("better-sqlite3/lib/sqlite-error") instead of
Database.SqliteError results in the same behavior in both cases.

I suspect that this has something to do with the fact that the errors in
question are instantiated from a native addon—

// src/objects/database.lzz@89579fd576e1:292
		isolate->ThrowException(v8::Local<v8::Function>::New(isolate, SqliteError)->NewInstance(OnlyContext, 2, args).ToLocalChecked());
// src/objects/database.lzz@89579fd576e1:295
	static v8::Persistent<v8::Function> SqliteError;

—but I certainly don’t know exactly what’s going on.

This is Node v9.7.1 (currently on macOS 10.13.3).

[JoshuaWise]

Jest causes all sorts of problems with native addons, because they mess with things like v8 contexts and other wizardry that can easily cause bugs with C++ code that would otherwise be perfectly valid. I would not consider this an issue with better-sqlite3, but with Jest itself. A more robust testing framework (such as mocha, the one used in the better-sqlite3 repository), does not cause such an issue.

[JoshuaWise]

For clarity, the reason Jest causes an issue is because it loads the better-sqlite3 JavaScript code multiple times, creating multiple instances of its exports (such as SqliteError), which are not considered equal to each other. However, C++ code cannot be loaded multiple times (because of the way dynamic linking works). So when a native addon uses both C++ code and JavaScript code in coordination (which is perfectly valid in Node.js), Jest creates an environment where it is no longer valid, by testing the same C++ objects against different instances of JavaScript objects.

[wchargin]

Thanks for the info. Let me make sure I’m understanding correctly the
description of “C++ code that would otherwise be perfectly valid.” I
could interpret this in two ways:

1. better-sqlite3 is adhering to the spec for the V8 Context,
   Isolate, etc. APIs, such that it behaves correctly as long as its
   clients also adhere to the spec. Jest is using the API improperly,
   causing problems with better-sqlite3 through no fault of the latter.

2. better-sqlite3 uses the V8 APIs in ways that work perfectly fine
   under the current implementations of Node. Jest invokes the APIs in
   a different but also permissible way that better-sqlite3 is not
   written to handle.

Which did you mean?

If case (1), then this definitely sounds like a problem with Jest. In
this case, I’d be happy to open an issue on their repository. (If you
have any additional details about what Jest should stop doing, that’d be
great, too.)

[JoshuaWise]

Case 2 is what I meant to convey.

You see, Case 1 is not even true for Node.js core. Although Node.js exposes APIs for dealing with V8 contexts, objects provided by Node.js itself do no cooperate with V8 contexts besides the main context. Therefore I think it's acceptable for better-sqlite3 to have the same behavior.

In all practical ways, better-sqlite3 will work just fine within another V8 context. The tests for this module are just very very specific. Jest assumes your tests won't be very specific, and they assume that by running your tests within another V8 context, everything will be fine. They're making a gamble, hoping that the code you write will still work, even though Node.js itself does not play along with V8 contexts.

better-sqlite3 is a Node.js module, not a V8 module, so I think the current behavior is acceptable.

[mnmkng]

I'm having the issues described in #195 as well. I understand that this may not be solvable, but could you elaborate why even when using the --runInBand jest option (single process, serial run) the tests fail?

I worked around the problem by setting npm test to jest file_name1 && jest file_name2 which works fine, but the fact that it does not work with --runInBand makes me wonder if there's something else going on.

The code in question: https://github.com/apifytech/apify-storage-local-js/tree/request-queues

Thanks.

[JoshuaWise]

@mnmkng I could be wrong, but I believe that even with --runInBand, Jest still creates a new V8 context for the tests to run in.

[mnmkng]

@JoshuaWise Is there any way around this? I need to integrate with a library that uses Jest for its tests and there's nothing I can do about it (at least not in a foreseeable future). I switched to better-sqlite3 from sqlite3 and could not be happier with the library. Just now, at the very end of the process, I plugged the new thing (code shared above) into the library (with Jest tests) and the tests have gone completely crazy.

I'd be happy to contribute. I'm not really capable of changing anything on the native module side, but maybe the JS part could be refactored to overcome the problem?

[JoshuaWise]

Probably the best way around this is to never check err instanceof SqliteError, but instead check err.constructor.name === 'SqliteError' in your tests.

[mnmkng]

@JoshuaWise Oh, sorry, I don't actually have an issue with the error instance but rather the one from the related issue I linked above: #195. My pragma calls fail:

[mnmkng]

I don't know why it did not occur to me before, but the fix was actually quite easy. Replaced the pragma calls with exec calls and everything works fine.

this.db.exec('PRAGMA journal_mode = WAL');
this.db.exec('PRAGMA foreign_keys = ON');

[JoshuaWise]

This is fixed in v7.0.0.