better-sqlite3 conflicts with jest isolation

[johannes-vogel]

better-sqlite3 initializes the error constructor only once.

better-sqlite3/lib/database.js

Lines 58 to 61 in 60763a0

	if (!addon.isInitialized) {
	addon.setErrorConstructor(SqliteError);
	addon.isInitialized = true;
	}

Jest isolates test runners with new global objects and therefore any raised error after the first run are not an instance of the global error object anymore.

In my opinion, the raised errors should also in test cases be an instance of the global error object.

I've added a minimal sample (bs-jest-issue.zip) to reproduce the issue. It includes two identical tests which only pass once.

• npm i
• npm t

[Prinzhorn]

#162 (comment)
#779 (comment)
nodejs/node#37988

Not sure if anything changed since then? I personally manually downgrade SqliteError like this:

function downgradeError(err) {
  if (err instanceof SqliteError) {
    let nativeErr = new Error(err.message);
    nativeErr.isSqliteError = true;
    nativeErr.code = err.code;
    nativeErr.stack = err.stack;
    err = nativeErr;
  }

  return err;
}

process.on('uncaughtException', (err) => {
  throw downgradeError(err);
});

process.on('unhandledRejection', (err) => {
  throw downgradeError(err);
});

(I'm not using Jest at all btw, this is just to make errors properly propagate to the error event of the Worker I'm using)

[Prinzhorn]

Looks like this is not related to Workers but only to #162 (comment)

So Joshua recommends expect(e.constructor.name === 'SqliteError').toBeTruthy() until Jest fixes their setup.

[johannes-vogel]

@Prinzhorn Thanks for the issue reference. I was looking for a similar issue but obviously my search skills are limited.
I fear that Jest claims this as a feature to isolate tests and there also will not change their behavior...

Anyways, this issue is a duplicate of #162.

[oklemenz2]

I think the solution expect(e.constructor.name === 'SqliteError').toBeTruthy() is not valid, as for example VError uses instanceof. So as consumer of VError we don't have the error validation under control. Furthermore we see, that Promises resolve or reject depending on if error is a "real" error or just an object. So just checking the constructor name may help for local cases, but they do not help in the overall code, respecting dependencies.

Knowing that C-code is only loaded once, and knowing that Jest creates different environments, the only valid solution is to fix better-sqlite3 to set the setErrorConstructor every time and not only the first time.

So instead of this

if (!addon.isInitialized) {
  addon.setErrorConstructor(SqliteError);
  addon.isInitialized = true;
}

there should be only

addon.setErrorConstructor(SqliteError);

By this, the C-module always gets the current JS Error Constructor and uses this, when creating Error objects.
This is not an expensive operation and can be performed every time the database is constructed.

We patched the library and it works perfectly fine. Would be good to have this in standard.

[oklemenz2]

I still don't agree, that this issue is solved.

So marking this as duplicate to another issue, which does NOT solve the issue, but just proposes a workaround which does not apply in most cases, is not a solution.

It's sad to hear, that this is now closed and won't be fixed, as there is a one-liner simple solution (see my last comment), that would perfectly work and fix the problem.

So many more user of this library would now run in the same issue.

Sadly, if this is not fixed, we would need to look again for other sqlite implementation options.

[JoshuaWise]

By this, the C-module always gets the current JS Error Constructor and uses this, when creating Error objects.

@oklemenz2 What do you mean by "current" JS Error Constructor? It's my understanding that the multiple environments that Jest creates can run concurrently. It seems to me that your fix would actually create a race condition, and if your fix is working, it's only by coincidence.

Either way, this would potentially break things for other users who are using multiple v8 contexts in a different way from Jest.

[oklemenz2]

Yes, I see, I guess it's not deterministic, although in our test-suite it works perfectly fine (all the time). Can understand that a fix may not be applicable to everybody, although I cannot imagine, what should break more, because the error object is already set wrongly (this or the other way for multiple tests)...

[oklemenz2]

I added a follow-up for Jest: jestjs/jest#15024