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
Docker based test dbs #3157
Conversation
44acc89
to
2906709
Compare
8e2deb5
to
4155e4c
Compare
3f8ed41
to
2d3502d
Compare
scripts/docker-compose.yml
Outdated
- 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: |
There was a problem hiding this comment.
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.
2d3502d
to
fcb48cf
Compare
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. |
d74ebdc
to
fa31550
Compare
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. |
test/integration/builder/updates.js
Outdated
@@ -70,6 +70,80 @@ module.exports = function(knex) { | |||
}); | |||
}); | |||
|
|||
it('should immediately return updated value for other connections when updating row to DB returns', function() { |
There was a problem hiding this comment.
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
And still gotta fix those linting issues on sunday. |
scripts/docker-compose.yml
Outdated
- POSTGRES_PASSWORD=knextest | ||
- POSTGRES_DB=knex_test | ||
waitpostgres: | ||
image: postgres:10-alpine |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
@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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be prodDependency?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be moved then
There was a problem hiding this comment.
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
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)... |
Yep... I'm rebasing back to top of master... now there are just bunch of conflict to fix 😬 |
…erything else to work again
…o make node oracledb package to work
…le update in transaction does not seem to work ACID
@elhigu I can resolve conflicts if you want. |
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). |
6e20bc2
to
ae2149b
Compare
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", |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate, there's pretest
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate with test:tape
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
047a990
to
1bf149d
Compare
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. |
… of it on different docker versions
Finally green! Should we merge this? |
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 . |
Probably will still fail, because travis might not have oracle client libs installed yet.
Also found out that: