feat: run shared tests #390

jannyHou · 2019-06-28T15:05:12Z

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

package-lock.json

bajtos

Nice start!

.travis.yml

bajtos · 2019-07-08T14:36:22Z

lib/mysql.js

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


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?

@bajtos See tests in https://github.com/strongloop/loopback-connector-mysql/blob/1849507e065985ea0242440979a073bcebe4682e/test/mysql.test.js#L14, I guess it's for the consistent behaviors across all connectors.

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?

@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

test/init.js

bajtos · 2019-07-08T14:39:38Z

test/datatypes.test.js

@@ -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 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?

bajtos · 2019-07-08T14:39:42Z

test/migration.test.js

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


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?

bajtos · 2019-07-08T14:39:47Z

test/mysql.test.js

@@ -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 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 · 2019-07-08T17:30:05Z

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

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

jannyHou · 2019-07-08T18:01:16Z

@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 · 2019-07-08T18:13:58Z

@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 · 2019-07-09T07:41:45Z

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 👍

jannyHou · 2019-07-09T14:42:45Z

@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).

feat: run shared tests

jannyHou requested review from b-admike, dhmlau and emonddr as code owners June 28, 2019 15:05

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

jannyHou mentioned this pull request Jul 3, 2019

[Test PR, do not review thanks]Refactor/test non juggler #391

Closed

2 tasks

jannyHou force-pushed the refactor/juggler-test branch 2 times, most recently from 892807d to faf3862 Compare July 4, 2019 20:13

rmg reviewed Jul 4, 2019

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

jannyHou force-pushed the refactor/juggler-test branch 3 times, most recently from 3e12745 to f3bc17b Compare July 5, 2019 20:58

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

bajtos reviewed Jul 8, 2019

View reviewed changes

jannyHou mentioned this pull request Jul 8, 2019

fix failed tests if any #392

Closed

2 tasks

bajtos approved these changes Jul 9, 2019

View reviewed changes

jannyHou force-pushed the refactor/juggler-test branch from 172acf5 to 2fe7d64 Compare July 9, 2019 14:51

feat: run shared tests

01c94b6

jannyHou force-pushed the refactor/juggler-test branch from 2fe7d64 to 01c94b6 Compare July 9, 2019 15:36

jannyHou merged commit f556b7c into master Jul 9, 2019

delete-merged-branch bot deleted the refactor/juggler-test branch July 9, 2019 16:01

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

Merge pull request loopbackio#390 from strongloop/refactor/juggler-test

8d1337a

feat: run shared tests

This was referenced Feb 11, 2020

Run shared tests from both v3 and v4 of juggler loopbackio/loopback-connector-db2#144

Merged

Run shared test from both v3 and v4 of juggler loopbackio/loopback-connector-dashdb#96

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: run shared tests #390

feat: run shared tests #390

jannyHou commented Jun 28, 2019 •

edited

Loading

bajtos left a comment

bajtos Jul 8, 2019

jannyHou Jul 8, 2019 •

edited

Loading

bajtos Jul 9, 2019

jannyHou Jul 9, 2019

bajtos Jul 8, 2019

bajtos Jul 8, 2019

bajtos Jul 8, 2019

jannyHou commented Jul 8, 2019 •

edited

Loading

jannyHou commented Jul 8, 2019

jannyHou commented Jul 8, 2019

bajtos commented Jul 9, 2019

jannyHou commented Jul 9, 2019

feat: run shared tests #390

feat: run shared tests #390

Conversation

jannyHou commented Jun 28, 2019 • edited Loading

Description

Related issues

Checklist

bajtos left a comment

Choose a reason for hiding this comment

bajtos Jul 8, 2019

Choose a reason for hiding this comment

jannyHou Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

bajtos Jul 9, 2019

Choose a reason for hiding this comment

jannyHou Jul 9, 2019

Choose a reason for hiding this comment

bajtos Jul 8, 2019

Choose a reason for hiding this comment

bajtos Jul 8, 2019

Choose a reason for hiding this comment

bajtos Jul 8, 2019

Choose a reason for hiding this comment

jannyHou commented Jul 8, 2019 • edited Loading

jannyHou commented Jul 8, 2019

jannyHou commented Jul 8, 2019

bajtos commented Jul 9, 2019

jannyHou commented Jul 9, 2019

jannyHou commented Jun 28, 2019 •

edited

Loading

jannyHou Jul 8, 2019 •

edited

Loading

jannyHou commented Jul 8, 2019 •

edited

Loading