Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

juggler-v3-v4 shared test #206

Merged
merged 1 commit into from
Jun 21, 2019
Merged

juggler-v3-v4 shared test #206

merged 1 commit into from
Jun 21, 2019

Conversation

agnes512
Copy link
Contributor

@agnes512 agnes512 commented Jun 17, 2019

Description

At the moment, our connectors are installing juggler 3.x to run the test suite. When we make a change to juggler@3, cis-jenkins detects downstream dependency and triggers CI build of connectors with the modified juggler version. When we make a change to juggler's master, no downstream builds are triggered.

In the past, we were maintaining multiple branches to test connectors against different juggler versions. E.g. loopback-2.x containing code from "master" but installing juggler 2.x for testing. This is rather impractical, because we don't have bandwidth to update those other branches with changes made to master.

In this pull request, I am reworking connector test setup to execute tests from both juggler versions 3.x and 4.x, which are triggered in ./test.js.

Drop Node.js 6 CI tests as Juggler v4 does not support it. Also update eslint to the latest version (see those 2 PRs below).

Related issues

References issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide



/** from couldant.connection.test */
describe('cloudant connection', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should stay in cloudant.connection.test.

The purpose of juggler-v*/test.js is to execute the shared test suite provided by juggler. Connector-specific tests are staying in test/* files and are executed against a single juggler version only. At least that have been our approach so far.

return global.resetDataSourceClass();
});
});
// keep this part?
describe('cloudant imported features', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this describe line, we already have describe(name,...) above - that should be enough.

It's also crucial to call require('loopback-datasource-juggler/test/*'); from inside describe(name,...) block!

describe(name, function() {
  before(function() {
    IMPORTED_TEST = true;
    return global.resetDataSourceClass(juggler.DataSource);
  });
	
  after(function() {
    IMPORTED_TEST = false;
    return global.resetDataSourceClass();
  });

  require('loopback-datasource-juggler/test/include.test.js');
  require('loopback-datasource-juggler/test/common.batch.js');
});


process.env.COUCHDB2_TEST_SKIP_INIT = true;

describe('cloudant automigrate.test - imported from couchdb2', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests imported from loopback-connector-couchdb2 should stay in test/imported.test.js, they are not part of deps/juggler-v* test suite.

});

after(function() {
return global.resetDataSourceClass();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this work, you need to modify test/init.js, add resetDataSourceClass helper and modify getDataSource to allow configuration of the data-source class used. See https://github.com/strongloop/loopback-connector-mongodb/pull/519/files#diff-40dfb1c53004f1488cbe6eb300b286dd

@agnes512
Copy link
Contributor Author

@slnode test please

@jannyHou
Copy link
Contributor

loopbackio/loopback-datasource-juggler#1750 should fix the 2 failures for the tests with juggler 4.x

@agnes512 agnes512 force-pushed the build/test-juggler-v3-and-v4 branch 4 times, most recently from 80c18fc to 8d1e93b Compare June 19, 2019 19:53
@agnes512 agnes512 marked this pull request as ready for review June 19, 2019 19:58
@agnes512
Copy link
Contributor Author

@slnode test please

});
require('loopback-datasource-juggler/test/include.test.js');
require('loopback-datasource-juggler/test/common.batch.js');
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a TODO comment to remind us that we should eventually include the following tests too:

  • loopback-datasource-juggler/test/default-scope.test.js
  • loopback-datasource-juggler/test/persistence-hooks.suite.js

See https://github.com/strongloop/loopback-connector-mongodb/blob/master/deps/juggler-v4/test.js

package.json Outdated
"eslint-config-loopback": "^4.0.0",
"loopback-datasource-juggler": "^3.0.0",
"eslint": "^5.1.0",
"eslint-config-loopback": "^10.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update of eslint-config-loopback causes a lot of reformatting (adds a lot of insignificant white-space changes), which makes it difficult to see what changes are relevant to the introduction of the shared tests.

Please open a new pull request to update eslint config to the latest version & fix linting problems. Once that PR is landed, rebase this PR on top of the new master so that it contains only changes relevant to the introduction of the shared tests.

@agnes512 agnes512 force-pushed the build/test-juggler-v3-and-v4 branch 3 times, most recently from b364cf9 to 98b11e3 Compare June 20, 2019 18:54
@agnes512 agnes512 force-pushed the build/test-juggler-v3-and-v4 branch from b23840b to 5fa0e51 Compare June 21, 2019 14:18
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@agnes512 agnes512 force-pushed the build/test-juggler-v3-and-v4 branch from 5fa0e51 to e0539a3 Compare June 21, 2019 14:44
Copy link
Contributor

@nabdelgadir nabdelgadir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@agnes512 agnes512 merged commit 0ffcd3c into master Jun 21, 2019
@delete-merged-branch delete-merged-branch bot deleted the build/test-juggler-v3-and-v4 branch June 21, 2019 17:51
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants