feat(repository-tests): run repository-tests on cloudant #3968

agnes512 · 2019-10-18T01:43:14Z

closes #3437

Create acceptance-repository-cloudant folder.

Modify tests in replace-by-id need to be modified to run the replacement operation on Cloudant ( it needs an extra property _rev to do so).
Set up Travis task: it needs to run the script(docker.setup.js) to set up the docker image.
the docker.setup.js file is based on @jannyHou 's PR. I modified it a bit, now it can be used to set up the local test as well with customized names. ( see README).
Add local test setup and README file to allow run repo tests on Cloudant ( or on docker) locally.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

bajtos

Nice start!

.travis.yml

acceptance/repository-cloudant/src/__tests__/cloudant-default-repository.acceptance.ts

agnes512 · 2019-11-13T16:44:04Z

packages/repository-tests/src/crud/replace-by-id.suite.ts

@@ -34,6 +34,11 @@ export function createSuiteForReplaceById(
    })
    id: MixedIdType;

+    // cloudant needs this property to do replacement method
+    // see cloundant README file for more details


Cloudant README

I find it suboptimal to force all connector tests (SQL, MongoDB, etc.) to use _rev property. Let's find a way how to allow individual connectors to provide any extra properties via CrudFeatures.

I see that you added revisionTokenRequired flag to fix test failures.

Personally, I'd prefer to find a way how to define _rev property only when running on Cloudant, so that we are not using _rev property when testing e.g. MongoDB.

Having said that, I don't have any solution I could offer from the top of my head, so it's probably better to accept the current proposal for now and get this pull request landed sooner rather than later 👍

acceptance/repository-cloudant/package.json

.travis.yml

bajtos

Great job, @agnes512 👏 I am happy to see that you managed to find a working solution for running our tests against Cloudant 👍

Let's discuss & improve the implementation details now.

bajtos · 2019-11-18T07:49:39Z

docker.setup.js

+// // we don't pass any node flags, so we can call _mocha instead the wrapper
+// const mochaBin = require.resolve('mocha/bin/_mocha');
+
+process.env.CLOUDANT_DATABASE = 'testdb';


Since this docker setup is specific to Cloudant, can you please move the setup script to acceptance/repository-cloudant?

This file will be removed once the release is done. The script is from #3968. I use it here to check if it sets up travis correctly and takes reasonable time to finish.

@bajtos Thank you for the technical feedback of the setup script, it's actually added in https://github.com/strongloop/loopback-connector-cloudant/blob/master/docker.setup.js, @agnes512 put it in her PR before that script merged into the master.

To be honest, to save the time, the code in that script is mostly copied from an existing script that has been running for test from 2 years ago 🙈 , so for some of your questions I don't quite know the answer, but I'll try :p

Thank you for the technical feedback of the setup script, it's actually added in https://github.com/strongloop/loopback-connector-cloudant/blob/master/docker.setup.js, @agnes512 put it in her PR before that script merged into the master.

I see, makes sense.

To be honest, to save the time, the code in that script is mostly copied from an existing script that has been running for test from 2 years ago

Sure, no need to refactor the script if it's working well as it is now. I am going to mark all my comments for docker.setup.js as resolved then.

docker.setup.js

bajtos · 2019-11-18T07:57:30Z

docker.setup.js

+async.waterfall(
+  [
+    dockerStart('ibmcom/couchdb3:latest'),
+    sleep(ms('2s')),


I am little bit concerned about the hard-coded delay. In many cases, the docker will start faster and we wait unnecessarily long. In some edge cases, the docker may not start fast enough, in which case the subsequent steps will fail (assuming they detect & handle this error correctly).

Is there any reasonable easy way how to replace this sleep with poll-based approach? E.g. every 100ms check if the docker container has started. If yes then the wait is over, if not then sleep 100ms and try again.

Feel free to leave this out of this pull request though.

docker.setup.js

bajtos · 2019-11-18T08:00:49Z

package.json

@@ -62,7 +66,8 @@
    "docs:prepare": "./docs/bin/build-preview-site.sh",
    "docs:start": "cd docs/_preview && bundle exec jekyll serve --no-w --i",
    "mocha": "node packages/build/bin/run-mocha \"packages/*/dist/__tests__/**/*.js\" \"extensions/*/dist/__tests__/**/*.js\" \"examples/*/dist/__tests__/**/*.js\" \"packages/cli/test/**/*.js\" \"packages/build/test/*/*.js\"",
-    "posttest": "npm run lint"
+    "posttest": "npm run lint",
+    "docker:setup": "node ./docker.setup.js"


Since the Docker setup is specific to Cloudant, can we move this npm script to acceptance/repository-cloudant/package.json please?

it will be loaded from dependency loopback-connector-cloudant.

@jannyHou I moved the whole script to the acceptance/repo-cloudant. But I am okay with either way~

bajtos · 2019-11-18T08:02:21Z

packages/repository-tests/src/crud/replace-by-id.suite.ts

@@ -34,6 +34,11 @@ export function createSuiteForReplaceById(
    })
    id: MixedIdType;

+    // cloudant needs this property to do replacement method
+    // see cloundant README file for more details


I find it suboptimal to force all connector tests (SQL, MongoDB, etc.) to use _rev property. Let's find a way how to allow individual connectors to provide any extra properties via CrudFeatures.

bajtos · 2019-11-18T08:06:41Z

packages/repository-tests/src/crud/replace-by-id.suite.ts

-      expect(toJSON(found)).to.deepEqual(
+      // use containDeep instead of deepEqual because cloudant changes _rev value after
+      // replacement
+      expect(toJSON(found)).to.containDeep(


I am concerned that by changing from deepEqual to containDeep we are allowing many more changes than just a different _rev value. For example, if the connector and/or the database adds new properties, this assertion will happily accept that, even though such behavior is most likely not wanted.

Let's find a way how to deal with _rev changes via a new CrudFeatures flag in such way that only changes in the _rev property is allowed and only when testing against Cloudant/CouchDB.

Updated in the fifth commit.

bajtos · 2019-11-18T08:06:47Z

packages/repository-tests/src/crud/replace-by-id.suite.ts

-      expect(toJSON(found)).to.deepEqual(
+      // use containDeep instead of deepEqual because cloudant changes _rev value after
+      // replacement
+      expect(toJSON(found)).to.containDeep(


bajtos · 2019-11-18T08:09:13Z

acceptance/repository-cloudant/README.md

+- Use the `couchDB3:latest` image, and rename the container as
+  `cloudant-testdb`.
+
+- Start the container, and create an admin and password for this image at


Can we automate this and the previous step please? In other repository acceptance packages, the docker-based setup requires the developer to run a single script only. Can we perhaps tell people to use npm docker:setup? (See also my other comments suggesting to move that script to this package.)

Done, and README is updated. ( the script is run in this folder now)

rmg · 2019-11-18T18:53:59Z

acceptance/repository-cloudant/docker.setup.js

+const docker = new require('dockerode')();
+const http = require('http');
+const ms = require('ms');
+const fs = require('fs-extra');


fs-extra isn't listed in package.json.. but it also doesn't appear to really be fully utilized here since the only method used is in fs from core.

true fs is enough

acceptance/repository-cloudant/package.json

bajtos

LGTM 👍

bajtos · 2019-11-19T13:02:54Z

acceptance/repository-cloudant/README.md


 ```bash
-source setup.sh <HOST> <PORT> <USER> <PASSWORD> <DATABASE>
+CLOUDANT_USER=<USER> CLOUDANT_PASSWORD=<PASSWORD> CLOUDANT_DATABASE=<DATABASE> node docker.setup.sh


node docker.setup.sh looks suspicious - node typically expects .js files, not .sh. I think it's just a typo?

Suggested change

CLOUDANT_USER=<USER> CLOUDANT_PASSWORD=<PASSWORD> CLOUDANT_DATABASE=<DATABASE> node docker.setup.sh

CLOUDANT_USER=<USER> CLOUDANT_PASSWORD=<PASSWORD> CLOUDANT_DATABASE=<DATABASE> node docker.setup.js

BTW why node docker.setup.js and not npm run docker:setup? Should we modify Travis CI config to call node docker.setup.js too?

Good catch!

And right, I think

CLOUDANT_USERNAME=<USER> CLOUDANT_PASSWORD=<PASSWORD> CLOUDANT_DATABASE=<DATABASE> npm run docker:setup

also allows users to have their own configurations 👍 changed.

bajtos · 2019-11-19T13:08:47Z

packages/repository-tests/src/crud/replace-by-id.suite.ts

@@ -34,6 +34,11 @@ export function createSuiteForReplaceById(
    })
    id: MixedIdType;

+    // cloudant needs this property to do replacement method
+    // see cloundant README file for more details


I see that you added revisionTokenRequired flag to fix test failures.

Personally, I'd prefer to find a way how to define _rev property only when running on Cloudant, so that we are not using _rev property when testing e.g. MongoDB.

Having said that, I don't have any solution I could offer from the top of my head, so it's probably better to accept the current proposal for now and get this pull request landed sooner rather than later 👍

bajtos · 2019-11-19T13:10:59Z

packages/repository-tests/src/types.repository-tests.ts

+   *
+   * Default: `false`
+   */
+  revisionTokenRequired: boolean;


Nitpick - feel free to ignore.

The _rev value is not always required, is it? For example, when creating a new model instance, the _rev must not be provided, at least AFAIK.

Maybe hasRevisionToken would be a better name?

nabdelgadir

LGTM 👍

acceptance/repository-cloudant/package.json

nabdelgadir · 2019-11-19T19:52:54Z

acceptance/repository-cloudant/src/__tests__/cloudant-default-repository.acceptance.ts

+  crudRepositoryTestSuite(
+    CLOUDANT_CONFIG,
+    // Workaround for https://github.com/microsoft/TypeScript/issues/31840
+    DefaultTransactionalRepository as CrudRepositoryCtor,


Since it doesn't support transactions, I think you should change this to DefaultCrudRepository like in the MongoDB one.

👍 Cloudant uses _rev to handle the update conflict, it doesn't handle transaction like sql dbs.

right, I just copied it from MySQL :p

packages/repository-tests/src/crud/replace-by-id.suite.ts

jannyHou

👍 Almost there! LGTM after addressing comment from Nora and Ryan.
On a second thought I agree on keeping the setup script in this repository. Easy to maintain. the cloudant connector still uses the old script.

acceptance/repository-cloudant/package.json

jannyHou · 2019-11-19T20:52:38Z

acceptance/repository-cloudant/src/__tests__/cloudant-default-repository.acceptance.ts

+  crudRepositoryTestSuite(
+    CLOUDANT_CONFIG,
+    // Workaround for https://github.com/microsoft/TypeScript/issues/31840
+    DefaultTransactionalRepository as CrudRepositoryCtor,


👍 Cloudant uses _rev to handle the update conflict, it doesn't handle transaction like sql dbs.

agnes512 mentioned this pull request Oct 18, 2019

update docker setup script loopbackio/loopback-connector-cloudant#223

Closed

2 tasks

agnes512 force-pushed the cloudant-repo-tests branch from 12502eb to 28a847e Compare October 18, 2019 01:57

bajtos suggested changes Oct 18, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

acceptance/repository-cloudant/src/__tests__/cloudant-default-repository.acceptance.ts Outdated Show resolved Hide resolved

agnes512 force-pushed the cloudant-repo-tests branch 7 times, most recently from 0b7f930 to 04b9553 Compare October 21, 2019 14:51

agnes512 force-pushed the cloudant-repo-tests branch from a113c72 to 7f9e923 Compare November 13, 2019 16:41

agnes512 commented Nov 13, 2019

View reviewed changes

agnes512 force-pushed the cloudant-repo-tests branch 7 times, most recently from aec5e27 to 037dce5 Compare November 15, 2019 18:12

rmg reviewed Nov 15, 2019

View reviewed changes

acceptance/repository-cloudant/package.json Outdated Show resolved Hide resolved

rmg reviewed Nov 15, 2019

View reviewed changes