Document wrong error being thrown

[corollari]

Recently I had a test fail with the following error:

{filepath}/node_modules/dynalite/index.js:263
      if (err) throw err
               ^

Error: Database is not open
    at maybeError ({filepath}/node_modules/subleveldown/leveldown.js:219:32)
    at SubIterator._next ({filepath}/node_modules/subleveldown/leveldown.js:29:7)
    at SubIterator.AbstractIterator.next ({filepath}/node_modules/subleveldown/node_modules/abstract-leveldown/abstract-iterator.js:31:8)
    at Iterator._next ({filepath}/node_modules/encoding-down/index.js:123:11)
    at Iterator.AbstractIterator.next ({filepath}/node_modules/abstract-leveldown/abstract-iterator.js:31:8)
    at ReadStream._read ({filepath}/node_modules/level-iterator-stream/index.js:24:18)
    at ReadStream.Readable.read ({filepath}/node_modules/level-iterator-stream/node_modules/readable-stream/lib/_stream_readable.js:456:10)
    at maybeReadMore_ ({filepath}/node_modules/level-iterator-stream/node_modules/readable-stream/lib/_stream_readable.js:592:12)
    at processTicksAndRejections (internal/process/task_queues.js:84:21)
    at runNextTicks (internal/process/task_queues.js:66:3)

It turned out that the underlying cause for this error was a get operation that referenced a non-existing record, so the error is completely misleading.

I haven't looked much into it but this seems to be a problem with an upstream dependency (I might be wrong here tho). In any case, it may be interesting to discuss what to do with this, given that misreferenced records is a pretty common error to make, and it took me a lot of time to finally discover what the real error was in this case.

Some proposals:

• Document it
• Make an issue upstream
• Catch the error and replace it with a more meaningful one

[freshollie]

I'm not sure what I can do to counteract this. The error comes from: https://github.com/mhart/dynalite/blob/4f3e9209df0df2406b6fea778c4e8fd06676d4f5/index.js#L258

The internals of Dynalite show that the DB is "closed" before the http connection is closed which seems to be the cause of the error: https://github.com/mhart/dynalite/blob/4f3e9209df0df2406b6fea778c4e8fd06676d4f5/index.js#L38

An option would be to submit a PR to tear down the HTTP connection before stopping the DB. Are you sure this wasn't caused by executing a query after the DB has been torn down? As with #11?

Even if we improved dynalite by tearing down the http connection first, the error will still persist in that you could execute a DB request at exactly the same time as the DB is being closed. But dynalite is only torn down once all tests are completed, so this should only be possible from a dangling execution.

I can point this out in the readme?

describe("my bad test", () => {
  it("executes dynamodb, but doesn't await the response", () => {
    db.put(...).promise()
  })
});

// error thrown: Error: Database is not open

[corollari]

Are you sure this wasn't caused by executing a query after the DB has been torn down? As with #11?

I've done some more testing and it seems that basic tests that run ddb.get on nonexisting items work properly, so my initial hypothesis is wrong. However, I've gone back to the original error that made me open this issue and I've been able to reproduce the Database is not open error with the following set up:

test('passes', async () => {
  await ddb.put(object1).promise()
  await ddb.put(object2).promise()
  expect(
    testedFunction()
  ).rejects.toThrowError();
});

test('fails with "Database is not open"', async () => {
  await ddb.put(object1).promise()
  //await ddb.put(object2).promise()
  expect(
    testedFunction()
  ).rejects.toThrowError();
});

From my understanding, the test runner should wait for the promise returned by testedFunction to resolve before tearing down the DB, is that assumption wrong? And, if so, how come the other test works fine? I know that these code samples are not perfect as they are not directly executable, but the actual code that triggered this error is heavily intertwined with non-open-source bussiness code. I will try to get an executable minimal example once I have some free time.

I can point this out in the readme?

That sounds great, apart from that we could catch that error and replace it with a more descriptive one, what are your thoughts on that?

[freshollie]

Your tests should be

test('passes', async () => {
  await ddb.put(object1).promise()
  await ddb.put(object2).promise()
  // return the promise
  return expect(
    testedFunction()
  ).rejects.toThrowError();
});

test('fails with "Database is not open"', async () => {
  await ddb.put(object1).promise()
  //await ddb.put(object2).promise()

  // or await it
  await expect(
    testedFunction()
  ).rejects.toThrowError();
});

You must await the expect(...).rejects

Your first test passes because the DB is not torn down until the second test finishes.

[corollari]

I applied your changes but I still seem to get the same error.

Your first test passes because the DB is not torn down until the second test finishes.

I separated the tests into two files and ran them one at a time, I just put them together on my comment to make it more clear, should have specified it.

Am I missing something? I'll keep working on constructing a minimal example

[freshollie]

How does your testedFunction work?

[freshollie]

Any updates @corollari?

[corollari]

You were right, it was all caused by a DB operation that happened after the test finished. A minimal version of the code at play is:

test('fails with "Database is not open"', async () => {
  await ddb.put(object1).promise()
  await ddb.put(object2).promise()

  await expect(
    async () => {
      const obj2 = ddb.get(object2).promise()
      const obj1 = ddb.get(object1).promise()
      await obj1
      ddb.put(object3)
      await obj2
    }
  ).rejects.toThrowError();
});

When obj2 exists everything works fine and the put(object3) operation would finish before tear-down but if obj2 doesn't exist then that await returns directly and the put operation fails.

Given that this has been resolved, should we close this issue or use it to discuss ways to improve this type of error messages?

[freshollie]

@corollari not really sure what I can do for these error messages, only option would be to create a PR upstream to allow dynalite errors to be dynamic, and allow us to pass a message which gives helpful ways to solve this error ("ensure that all your DB calls happen before your tests finish", or something like that)