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

Docker based test dbs #3157

Merged
merged 16 commits into from May 13, 2019
Merged

Docker based test dbs #3157

merged 16 commits into from May 13, 2019

Conversation

@elhigu
Copy link
Member

@elhigu elhigu commented Apr 22, 2019

Probably will still fail, because travis might not have oracle client libs installed yet.

Also found out that:

  • current test suite is not closing down all started databases (so mocha must be ran with --exit switch)
  • acquireConnectionTimeout doesn't work for some reason with oracledb
scripts/docker-compose.yml Outdated Show resolved Hide resolved
@elhigu elhigu force-pushed the docker-based-test-dbs branch from 44acc89 to 2906709 Apr 22, 2019
test/unit/dialects/oracledb.js Outdated Show resolved Hide resolved
@elhigu elhigu force-pushed the docker-based-test-dbs branch 2 times, most recently from 8e2deb5 to 4155e4c Apr 22, 2019
.travis.yml Outdated Show resolved Hide resolved
test/unit/knex.js Show resolved Hide resolved
@elhigu elhigu force-pushed the docker-based-test-dbs branch 9 times, most recently from 3f8ed41 to 2d3502d May 9, 2019
- bash
- -c
- 'until /opt/oracle/product/18c/dbhomeXE/bin/sqlplus -s sys/Oracle18@oracledbxe/XE as sysdba <<< "SELECT 13376411 FROM DUAL; exit;" | grep "13376411"; do echo "Could not connect to oracle... sleep for a while"; sleep 5; done'
# testrunner:
Copy link
Member Author

@elhigu elhigu May 9, 2019

Choose a reason for hiding this comment

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

I was able to run oracle tests with this locally by copying needed client libs from oracledb container, so if nothing else works I'll add one more container, which actually runs the tests instead of travis trusty.

@elhigu elhigu force-pushed the docker-based-test-dbs branch from 2d3502d to fcb48cf May 9, 2019
@elhigu
Copy link
Member Author

@elhigu elhigu commented May 9, 2019

Only potential problem still here is that mysql server startup hangs some times... I'll try to rerun these to check out if that is still a problem.

@elhigu elhigu force-pushed the docker-based-test-dbs branch 4 times, most recently from d74ebdc to fa31550 May 9, 2019
@elhigu
Copy link
Member Author

@elhigu elhigu commented May 9, 2019

Yep, still various tests are unstable with oracledb (so far I've catched 3 when running tests in loop)... I'll skip them on oracle and add an issue about them. Still seems like the problem is that the way how knex does transactions with oracle is not completely stable...

Or we it might be less strict about when changes in DB is available for read for other connections when updating row in another... I really would like to write more multiconnection / concurrent reads / writes tests to be able to catch and reproduce those problems.

@@ -70,6 +70,80 @@ module.exports = function(knex) {
});
});

it('should immediately return updated value for other connections when updating row to DB returns', function() {
Copy link
Member Author

@elhigu elhigu May 10, 2019

Choose a reason for hiding this comment

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

This is the new test case... if we can setup knex oracledb dialect to work in a way that this tests passes, then probably most of the other failing oracle tests will start to work

@elhigu
Copy link
Member Author

@elhigu elhigu commented May 10, 2019

And still gotta fix those linting issues on sunday.

- POSTGRES_PASSWORD=knextest
- POSTGRES_DB=knex_test
waitpostgres:
image: postgres:10-alpine
Copy link
Collaborator

@kibertoad kibertoad May 10, 2019

Choose a reason for hiding this comment

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

why do we need two different postgres images?

Copy link
Member Author

@elhigu elhigu May 12, 2019

Choose a reason for hiding this comment

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

oops

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented May 11, 2019

@elhigu Merged changes from master that enabled running tests on Node 12. However, Oracle driver still doesn't support Node 12. Can we not execute Oracle tests on it?

package.json Outdated
@@ -20,11 +20,13 @@
"liftoff": "3.1.0",
"lodash": "^4.17.11",
"mkdirp": "^0.5.1",
"oracledb": "^3.1.2",
Copy link
Collaborator

@kibertoad kibertoad May 11, 2019

Choose a reason for hiding this comment

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

should it be prodDependency?

Copy link
Member Author

@elhigu elhigu May 12, 2019

Choose a reason for hiding this comment

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

nope, we don't have any of the drivers as knex deps to be able to install only one that is used in the project.

Copy link
Collaborator

@kibertoad kibertoad May 12, 2019

Choose a reason for hiding this comment

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

should be moved then

Copy link
Collaborator

@kibertoad kibertoad May 12, 2019

Choose a reason for hiding this comment

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

oracledb is still in prod dependencies :D

@elhigu
Copy link
Member Author

@elhigu elhigu commented May 12, 2019

@elhigu Merged changes from master that enabled running tests on Node 12. However, Oracle driver still doesn't support Node 12. Can we not execute Oracle tests on it?

No, oracle tests cannot be ran with node 12 yet. Also looks like that something else came with you merge here, since there is new failing test (TypeError: qb(...).table(...).having(...).clearHaving is not a function)...

@elhigu
Copy link
Member Author

@elhigu elhigu commented May 12, 2019

@elhigu Pull changes from master, I would expect that to fix clearHaving problem

Yep... I'm rebasing back to top of master... now there are just bunch of conflict to fix 😬

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented May 12, 2019

@elhigu I can resolve conflicts if you want.

@elhigu
Copy link
Member Author

@elhigu elhigu commented May 12, 2019

Looks like all of them was for package.json + travis (but had to fix them on every commit). You could check afterwards that they look fine after I'll update this branch (I might have lost some of the changes / package version there).

@elhigu elhigu force-pushed the docker-based-test-dbs branch from 6e20bc2 to ae2149b May 12, 2019
package.json Outdated
"build": "npm run babel",
"debug:test": "node --inspect-brk ./node_modules/.bin/_mocha -- --exit -t 0 test/index.js",
"debug_test": "mocha --inspect-brk --exit -t 0 test/index.js",
Copy link
Collaborator

@kibertoad kibertoad May 12, 2019

Choose a reason for hiding this comment

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

duplicate, there's already "debug:test"

package.json Outdated
"plaintest:sqlite": "cross-env DB=sqlite3 npm run plaintest",
"prepare": "npm run babel",
"prepublishOnly": "npm run babel",
"pretest": "npm run lint && npm run lint:types && npm run babel",
"test": "nyc mocha --exit --check-leaks --globals __core-js_shared__ -t 10000 -R spec test/index.js && npm run test:tape && npm run test:cli",
"test": "nyc mocha --exit --check-leaks --globals __core-js_shared__ -t 10000 test/index.js && npm run tape && npm run bin_test",
Copy link
Collaborator

@kibertoad kibertoad May 12, 2019

Choose a reason for hiding this comment

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

Should be "npm run test:tape && npm run test:cli""

package.json Outdated
"pre_test": "npm run lint && npm run lint_types",
"bin_test": "cross-env KNEX_PATH=../knex.js KNEX=bin/cli.js jake -f test/jake/Jakefile",
"tape": "node test/tape/index.js | tap-spec",
"debug_tape": "node --inspect-brk test/tape/index.js",
Copy link
Collaborator

@kibertoad kibertoad May 12, 2019

Choose a reason for hiding this comment

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

duplicate, there's already "debug:tape"

package.json Outdated
@@ -103,6 +107,13 @@
"mssql:test": "DB=mssql npm run plaintest",
"mssql:destroy": "docker-compose -f scripts/mssql-docker-compose.yml stop",
"postmssql:init": "node scripts/wait-for-mssql-server.js && npm run mssql:logs || (npm run mssql:logs;false)",
"prepublish": "npm run babel",
"pre_test": "npm run lint && npm run lint_types",
Copy link
Collaborator

@kibertoad kibertoad May 12, 2019

Choose a reason for hiding this comment

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

duplicate, there's pretest

Copy link
Member Author

@elhigu elhigu May 12, 2019

Choose a reason for hiding this comment

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

is pretest run automatically before publish?

Copy link
Member Author

@elhigu elhigu May 12, 2019

Choose a reason for hiding this comment

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

aa you meant pre_test 👍

package.json Outdated
@@ -103,6 +107,13 @@
"mssql:test": "DB=mssql npm run plaintest",
"mssql:destroy": "docker-compose -f scripts/mssql-docker-compose.yml stop",
"postmssql:init": "node scripts/wait-for-mssql-server.js && npm run mssql:logs || (npm run mssql:logs;false)",
"prepublish": "npm run babel",
"pre_test": "npm run lint && npm run lint_types",
"bin_test": "cross-env KNEX_PATH=../knex.js KNEX=bin/cli.js jake -f test/jake/Jakefile",
Copy link
Collaborator

@kibertoad kibertoad May 12, 2019

Choose a reason for hiding this comment

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

duplicate with test:cli

package.json Outdated
"prepublish": "npm run babel",
"pre_test": "npm run lint && npm run lint_types",
"bin_test": "cross-env KNEX_PATH=../knex.js KNEX=bin/cli.js jake -f test/jake/Jakefile",
"tape": "node test/tape/index.js | tap-spec",
Copy link
Collaborator

@kibertoad kibertoad May 12, 2019

Choose a reason for hiding this comment

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

duplicate with test:tape

Copy link
Member Author

@elhigu elhigu May 12, 2019

Choose a reason for hiding this comment

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

how about that "prepublishOnly", when that is ran?

Copy link
Collaborator

@kibertoad kibertoad May 12, 2019

Choose a reason for hiding this comment

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

npm is throwing deprecation warning about prepublish now. Now there are two separate hooks: prepublishOnly for publish and prepare for after install. prepublish should actually be removed, next major npm version won't support it.

Copy link
Member Author

@elhigu elhigu May 12, 2019

Choose a reason for hiding this comment

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

thanks, I'll remove prepublish already then. Its really unlikely that we would publish anything without running babel first, since its pretty much included in every hook there :)

@elhigu elhigu force-pushed the docker-based-test-dbs branch from 047a990 to 1bf149d May 12, 2019
@elhigu
Copy link
Member Author

@elhigu elhigu commented May 12, 2019

Again something really strange happens in travis that doesn't happen locally... created another simpler project to explore that https://github.com/elhigu/travis-node-oracle-18-xe

Basically locally installed libs are found in /usr/lib/oracle... inside docker container, but in travis that directory doesn't seem to be created, even that both uses the same docker image where to install them.

@kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented May 12, 2019

Finally green! Should we merge this?

package.json Outdated Show resolved Hide resolved
@elhigu elhigu merged commit 8a9a648 into knex:master May 13, 2019
2 checks passed
@elhigu
Copy link
Member Author

@elhigu elhigu commented May 13, 2019

huh finally its over.... I saw one random hanging after tests, which might mean that some connection is left open in some cases. or pool cannot tear down properly. If that happens we could add flag that forces mocha to exit even if there are connections left open and see if stuff works better when tests are written in a more clean manner with async / await .

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

Successfully merging this pull request may close these issues.

None yet

2 participants