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

Conversation

Projects
None yet
2 participants
@elhigu
Copy link
Collaborator

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
Show resolved Hide resolved scripts/docker-compose.yml Outdated

@elhigu elhigu force-pushed the elhigu:docker-based-test-dbs branch from 44acc89 to 2906709 Apr 22, 2019

Show resolved Hide resolved test/unit/dialects/oracledb.js Outdated

@elhigu elhigu force-pushed the elhigu:docker-based-test-dbs branch 2 times, most recently from 8e2deb5 to 4155e4c Apr 22, 2019

Show resolved Hide resolved .travis.yml Outdated

@elhigu elhigu force-pushed the elhigu:docker-based-test-dbs branch 9 times, most recently from 3f8ed41 to 2d3502d May 4, 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:

This comment has been minimized.

Copy link
@elhigu

elhigu May 9, 2019

Author Collaborator

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 elhigu:docker-based-test-dbs branch from 2d3502d to fcb48cf May 9, 2019

@elhigu

This comment has been minimized.

Copy link
Collaborator Author

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 elhigu:docker-based-test-dbs branch 4 times, most recently from d74ebdc to fa31550 May 9, 2019

@elhigu

This comment has been minimized.

Copy link
Collaborator Author

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() {

This comment has been minimized.

Copy link
@elhigu

elhigu May 10, 2019

Author Collaborator

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

This comment has been minimized.

Copy link
Collaborator Author

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

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 10, 2019

Collaborator

why do we need two different postgres images?

This comment has been minimized.

Copy link
@elhigu

elhigu May 12, 2019

Author Collaborator

oops

@kibertoad

This comment has been minimized.

Copy link
Collaborator

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?

@@ -20,11 +20,13 @@
"liftoff": "3.1.0",
"lodash": "^4.17.11",
"mkdirp": "^0.5.1",
"oracledb": "^3.1.2",

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 11, 2019

Collaborator

should it be prodDependency?

This comment has been minimized.

Copy link
@elhigu

elhigu May 12, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 12, 2019

Collaborator

should be moved then

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 12, 2019

Collaborator

oracledb is still in prod dependencies :D

@elhigu

This comment has been minimized.

Copy link
Collaborator Author

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

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented May 12, 2019

@elhigu I can resolve conflicts if you want.

@elhigu

This comment has been minimized.

Copy link
Collaborator Author

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 elhigu:docker-based-test-dbs branch from 6e20bc2 to ae2149b May 12, 2019

"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",

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 12, 2019

Collaborator

duplicate, there's already "debug:test"

"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",

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 12, 2019

Collaborator

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

"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",

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 12, 2019

Collaborator

duplicate, there's already "debug:tape"

@@ -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",

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 12, 2019

Collaborator

duplicate, there's pretest

This comment has been minimized.

Copy link
@elhigu

elhigu May 12, 2019

Author Collaborator

is pretest run automatically before publish?

This comment has been minimized.

Copy link
@elhigu

elhigu May 12, 2019

Author Collaborator

aa you meant pre_test 👍

@@ -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",

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 12, 2019

Collaborator

duplicate with test:cli

"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",

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 12, 2019

Collaborator

duplicate with test:tape

This comment has been minimized.

Copy link
@elhigu

elhigu May 12, 2019

Author Collaborator

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

This comment has been minimized.

Copy link
@kibertoad

kibertoad May 12, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@elhigu

elhigu May 12, 2019

Author Collaborator

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 elhigu:docker-based-test-dbs branch from 047a990 to 1bf149d May 12, 2019

@elhigu

This comment has been minimized.

Copy link
Collaborator Author

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

This comment has been minimized.

Copy link
Collaborator

commented May 12, 2019

Finally green! Should we merge this?

Show resolved Hide resolved package.json Outdated

@elhigu elhigu merged commit 8a9a648 into tgriesser:master May 13, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+4.3%) to 89.565%
Details
@elhigu

This comment has been minimized.

Copy link
Collaborator Author

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
You can’t perform that action at this time.