Modernize and fix with recent versions of PouchDB.

[loic]

No description provided.

[marten-de-vries]

I just restarted the failing tests for the third time to be sure, but I think the failures might actually be genuine.

[loic]

Sorry @marten-de-vries, I had a green build at the time of the PR, but I then realized a few things:

• I only upgraded pouchdb to 6.4 in package.json, but the test server actually uses pouchdb-core directly which was still at version 6.1.
• I didn't upgrade all the other packages that live on the monorepo to 6.4.
• The pouchdb test suite was the old one due to the pinned commit in https://github.com/pouchdb/pouchdb-server/blob/master/bin/test-setup.sh#L18.

That meant that plugins that live in the monorepo were properly tested against 6.4 since they require the pouchdb package, but the legacy pouchdb test suite was running against the legacy pouchdb-core version, which while it worked, defeated the purpose of modernizing the repo.

Now I've pinned everything to 6.4, including the test suite, but since I've done that I've been going deeper and deeper down the rabbit-hole. Hopefully I've just managed to fix the sqlite build. Now I've got to figure out what's going on with test-express-minimum.

[marten-de-vries]

Nice work. I looked into why test-express-minimum fails, as it surprised me, and I think it's just a mistake in the test suite implementation:

• The test that fails tries to cop out if it's not running in a fully fledged CouchDB (with validation etc.): https://github.com/pouchdb/pouchdb/blob/master/tests/integration/test.bulk_docs.js#L402
• express-pouchdb in the minimum mode is not a fully fledged CouchDB, but the function the test suite uses to check for that thinks it is: https://github.com/pouchdb/pouchdb/blob/master/tests/integration/utils.js

Not sure what the easiest way to actually fix this is.

[loic]

I'm really puzzled by those test-express-minimum failures, the test suite fails in really weird ways.

First things start to go sour after a timeout, but since the timeout is always reproducible and doesn't seem time dependent (I tried to vary the timeout duration) I guess it's due to a deadlock.

Second, the test suite can't recover from that failure: I modify test-node.sh to remove the --bail option of mocha, by as soon as this failure happen mocha aborts.

I commented this failing test.bulk_docs.js test, and another test fails with a timeout in test.replication.js, and that's where things become hard to conceive. Commenting that new failing test makes the test immediately before it fail even though it was passing previously. Commenting that new failing test as well makes the one even before that fail, I've commented half a dozen tests before giving up. I'm having a hard time understanding how commenting a test makes a test that executes before it fail... I'm suspecting it may have sometime to do with beforeEach/afterEach routines.

Commenting the whole test.replication.js as well as the one failing test in test.bulk_docs.js, leads to 2 other failing tests, but at least those looks like more reasonable failures:

  1) test.sync.js-local-http #3270 triggers "denied" events:

      AssertionError: expected 0 to equal 2
      + expected - actual

      -0
      +2

      at tests/integration/test.sync.js:650:38
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:160:7)

  2) test.sync.js-local-http #3270 triggers "denied" events, reverse direction:

      AssertionError: expected 0 to equal 2
      + expected - actual

      -0
      +2

      at tests/integration/test.sync.js:696:40
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:160:7)

[loic]

Regarding the CouchDB bulk_docs.js deadlock: https://github.com/pouchdb/pouchdb/blob/master/tests/integration/test.bulk_docs.js#L453 should be .then(done, done) so the test can fail gracefully.

At least now we have an actual failure:

  1) test.bulk_docs.js-http #2935 new_edits=false with single unauthorized:
     AssertionError: expected [] to have a length of 1 but got 0
      at tests/integration/test.bulk_docs.js:451:27
      at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:160:7)

[loic]

express-pouchdb in the minimum mode is not a fully fledged CouchDB, but the function the test suite uses to check for that thinks it is
Not sure what the easiest way to actually fix this is.

If the pouchdb test suite now requires some of those functionalities, maybe we should activate them by default in the modes.minimumForPouchDB mode? Note that I tried adding validation (the reason test.bulk_docs.js actually fails) to the modes.minimumForPouchDB array but that didn't seem to work, I guess I'm missing other roles like routes, changing the mode in express-pouchdb-minimum-for-pouchdb.js to fullCouchDB worked however, and also addressed all other failing tests.

If we want to preserve backwards compatibility we could only change the test instance of express-pouchdb, but then we'd make the README lie a little: "minimumForPouchDB [...] This is the minimum required to make the PouchDB test suite run".

At least we are seeing the light at the end of the tunnel, we just need to turn my last commit into the preferred option, wdyt @marten-de-vries?

[loic]

The following works:

'use strict';

var PouchDB = require('pouchdb');

var app = require('../packages/node_modules/express-pouchdb')(PouchDB, {
  mode: 'minimumForPouchDB',
  overrideMode: {
    'include': [
      'routes/replicate',
      'validation'
    ]
  }
});
app.listen(6984);

I'm not sure about validation, but routes/replicate make sense to me to have as a default mode (i.e. in modes.minimumForPouchDB not in express-pouchdb-minimum-for-pouchdb.js).

[marten-de-vries]

I think the proper fix would be to change isCouchDB to something that instead detects what it is used for: whether validation is supported. Renaming the function would be a nice bonus. We should probably open an issue for that on the pouchdb repo.

But as that would take at least another release to be usable, I'm fine with whatever workaround you come up with for now. Including enabling a few extra includes.

[loic]

CI doesn't seem to pick up changes.

[loic]

The description of minimumForPouchDB is the following:

'minimumForPouchDB': just exposes parts of the HTTP API that map 1-1 to the PouchDB api. This is the minimum required to make the PouchDB test suite run, and a nice start when you just need an HTTP API to replicate with.

I went with the hybrid approach, I changed the modes.minimumForPouchDB mode to add the replications routes, since it seems to me it qualifies as parts of the HTTP API that map 1-1 to the PouchDB api, and I added validation to the test runner in the interim of a fix in the pouchdb test suite.

[marten-de-vries]

Actually, _replicate is not required for passive replication, only _revs_diff is. That test just uses APIs that were not meant to be used with pouchdb-express-router/[express-pouchdb in minimum mode]. It detects the first properly, but not the second using isCouchDB.

Anyway, we can fix that later. I'm going to merge this.

[marten-de-vries]

Thanks for all your hard work on this, @loic!

[loic]

@marten-de-vries thanks for the merge!

Seems like I jumped to conclusions too quickly re the replication stuff. I'm gonna make an additional 3 PRs:

• 1 against pouchdb-server to revert the mode change, and opt for the same interim fix as for validation. This PR can be merged right away.
• 1 against pouchdb-server which introduces a new key on the welcome view, so we advertise the express-pouchdb-mode.
• 1 tentative fix against pouchdb which uses that new key in the function that performs the couchdb detection.

[marten-de-vries]

That sounds like a great solution.