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

feat: run shared tests #390

Merged
merged 1 commit into from Jul 9, 2019

Conversation

@jannyHou
Copy link
Contributor

commented Jun 28, 2019

Description

connect to #386

Run shared tests from juggler v3 and v4.

Related issues

#386

Checklist

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

@jannyHou jannyHou requested review from b-admike, dhmlau and emonddr as code owners Jun 28, 2019

@jannyHou jannyHou changed the title feat: run shared tests [WIP]feat: run shared tests Jun 28, 2019

@jannyHou jannyHou referenced this pull request Jul 3, 2019
0 of 2 tasks complete

@jannyHou jannyHou force-pushed the refactor/juggler-test branch 2 times, most recently from 892807d to faf3862 Jul 3, 2019

package-lock.json Outdated Show resolved Hide resolved

@jannyHou jannyHou force-pushed the refactor/juggler-test branch 3 times, most recently from 3e12745 to f3bc17b Jul 4, 2019

@jannyHou jannyHou changed the title [WIP]feat: run shared tests feat: run shared tests Jul 8, 2019

@bajtos
Copy link
Member

left a comment

Nice start!

.travis.yml Show resolved Hide resolved
@@ -444,6 +444,12 @@ MySQL.prototype.fromColumnValue = function(prop, val) {
lat: val.y,
};
break;
case 'ObjectID':

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 8, 2019

Member

Interesting. Why do we need to deal with ObjectID, considering that it's a type specific to MongoDB? Is this related to changes made in juggler v4?

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 8, 2019

Author Contributor

@bajtos See tests in

function ObjectID(id) {
, I guess it's for the consistent behaviors across all connectors.

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 9, 2019

Member

I see. test/mysql.test.js is not part of the shared test suite imported from juggler, so why are we fixing it in this pull request? Can you please open a new pull requests to fix problems that are unrelated to the shared test suite first?

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jul 9, 2019

Author Contributor

@bajtos Interesting 🤔 I thought you have noticed that after configuring the shared tests, the connector tests run with juggler v4(which this PR does) instead of juggler v3(the current master), and that's how all these fixes happen.
And I thought "running connector tests with juggler v4" is what we intend to do...

lib/mysql.js Show resolved Hide resolved
test/init.js Outdated Show resolved Hide resolved
@@ -206,7 +207,8 @@ describe('MySQL specific datatypes', function() {
var xcor, ycor;
City.create(city1, function(err, res) {
if (err) return done(err);
res.loc.should.deepEqual(city1.loc);
const loc_in_geo_type = new GeoPoint(city1.loc);
res.loc.should.deepEqual(loc_in_geo_type);

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 8, 2019

Member

This change seems unrelated to the introduction of shared test suite from juggler v4. Is it needed to fix failing tests? Can we make it in a standalone PR please?

cb(err, fields);
// The returned data are in arrays of type `RowDataPacket`,
// which are not objects.
cb(err, JSON.parse(JSON.stringify(fields)));

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 8, 2019

Member

This change seems unrelated to the introduction of shared test suite from juggler v4. Is it needed to fix failing tests? Can we make it in a standalone PR please?

@@ -88,7 +89,7 @@ describe('mysql', function() {

p.content.should.be.equal(post.content);
p.title.should.be.equal('a');
p.comments.should.eql(['1', '2']);
p.comments.should.eql(new List(['1', '2']));

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 8, 2019

Member

This change seems unrelated to the introduction of shared test suite from juggler v4. Is it needed to fix failing tests? Can we make it in a standalone PR please?

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

#390 (comment)
#390 (comment)
#390 (comment)

I submitted another PR to verify are they working with juggler v3 in #392

@jannyHou jannyHou referenced this pull request Jul 8, 2019
0 of 2 tasks complete
@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@bajtos Those 3 tests are related to the v3->v4 upgrade of juggler, see PR #392, so those fixes should still be kept in this PR.

I think v4 has a breaking change for the type coercion.

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

@bajtos Thank you for the review, replied.

I am ok to release a new major version for upgrading to juggler v4. For the failures on the current master, I prefer to leave it out of the scope of issue #386, we can either reopen #378 or create a new task for it :)

@bajtos

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

I am not sure if we need a new major version, leave that up to you to decide.

I think I am starting to understand what's the problem here. We are not only adding the shared test suite from juggler v4, but also upgrading the mysql-connector-specific test suite to execute against juggler v4. On master, these connector-specific tests are executed against juggler v3.

In that case it makes sense to keep the changes in test/* files as part of this pull request 👍

@bajtos

bajtos approved these changes Jul 9, 2019

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@bajtos

I think I am starting to understand what's the problem here. We are not only adding the shared test suite from juggler v4, but also upgrading the mysql-connector-specific test suite to execute against juggler v4. On master, these connector-specific tests are executed against juggler v3.

Ah yes! Then never mind about my comment #390 (comment).

@jannyHou jannyHou force-pushed the refactor/juggler-test branch from 172acf5 to 2fe7d64 Jul 9, 2019

@jannyHou jannyHou force-pushed the refactor/juggler-test branch from 2fe7d64 to 01c94b6 Jul 9, 2019

@jannyHou jannyHou merged commit f556b7c into master Jul 9, 2019

16 checks passed

Commit Linter commits are all properly formatted
Details
PR Linter PR is up to date
Details
[cis-jenkins] PR Builder Build finished.
Details
[cis-jenkins] downstream: loopback-getting-started-intermediate@master Success! (01c94b6)
Details
[cis-jenkins] downstream: loopback-getting-started@master Success! (01c94b6)
Details
[cis-jenkins] x64 && linux && nvm && dbs,10 Success! (01c94b6)
Details
[cis-jenkins] x64 && linux && nvm && dbs,6 Success! (01c94b6)
Details
[cis-jenkins] x64 && linux && nvm && dbs,8 Success! (01c94b6)
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
loopback-connector-mysql Success! (01c94b6)
Details
loopback-connector-mysql/node=4.x,os=windows Success! (01c94b6)
Details
loopback-connector-mysql/node=6.x,os=windows Success! (01c94b6)
Details
pr-builder
Details
security/snyk - package.json (StrongLoop) No new issues
Details

@delete-merged-branch delete-merged-branch bot deleted the refactor/juggler-test branch Jul 9, 2019

loryk pushed a commit to loryk/loopback-connector-mysql that referenced this pull request Aug 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.