Fix datatypeChanged for fields with length

[joostdebruijn]

Description

The function datatypeChanged() in migration.js is always returning true, because the length was not evaluated properly. This pull request will fix that issue causing isActual() to work properly.

Related issues

103

Checklist

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

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[jannyHou]

@slnode test please

[joostdebruijn]

Hi @jannyHou, of course, but there are no tests for isActual() at all. I can add a specific testcase for what I've fixed, but maybe there should be added generic tests in the first place?

[joostdebruijn]

@jannyHou The test results are a bit weird, the tests ran succesfully on most OS'es, but are failing in Node 6.x (CI, Windows) and, throwing some Oracle-errors. Is there an explanation for this behaviour of the CI?

[jannyHou]

@joostdebruijn I think autoupdate is the function affected, the sequence of the calls is: autoupdate --> alterTable --> getAddModifyColumns --> getPropertiesToModify

So you can add or modify tests in https://github.com/joostdebruijn/loopback-connector-oracle/blob/7c7d1bebe5342e8a31d006d951f7c8844cf5d59b/test/oracle.autoupdate.test.js

[jannyHou]

And BTW, the change has 28 failures on CI, could you also run tests locally and fix them? Thanks~

[joostdebruijn]

@jannyHou I ran the tests twice, see results below. Because the results are identical before and after my change, I assume that my change is not breaking any existing test case. I'm absolutely willing to write a test case for my changes, but I don't have the time to fix other unrelated issues with the test suite or the CI. Also, please note the CI is giving positive results for some configurations, while it is failing on other configs and as far as I can see the logs, those issues are not related to my change - even the master branche is failing on the CI on 30 tests.

I'll commit a test case in the next few days, if you want, and I hope you're willing to accept my pull request thereafter.

Configuration: node 9.3.0 x64 npm 5.6.0 windows 10.0.16299.125 x64 oracle 12c Standard Edition Release 12.1.0.2.0 x64

Before my changes:

After my changes:

[jannyHou]

@joostdebruijn Sorry I was not realized that the master branch also fails, then don't worry about them. I will review your change and tests in this PR 👍

[joostdebruijn]

@jannyHou I just added one additional test case to verify that isActual() is working correctly and I extended one testcase to check if the length of a field is correctly changed when the model changes. I think this should be enough to cover the changes I made.

[joostdebruijn]

@jannyHou Have you had the opportunity to review this PR?

[joostdebruijn]

Is there a maintainer willing to review this pull request?

[kjdelisle]

@slnode test please

[kjdelisle]

@jannyHou Has the code been changed to your satisfaction (having previously reviewed)?

[joostdebruijn]

Any updates on this one? It is not really motivating to submit patches if it takes such a long time to review them...

[joostdebruijn]

🎉 Thanks, @jannyHou !