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.

Sign up for GitHub

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

Jump to bottom

Fix knex.initialize() and adds test #2477

Merged
merged 7 commits into from
Jul 2, 2018
Merged

Fix knex.initialize() and adds test #2477

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d4c038d
Fix knex.initialize() and adds test
pablopen Feb 16, 2018
3e1f0ec
Fix knex.initialize() and adds test
pablopen Feb 16, 2018
fd53ac7
Merge branch 'fix_client_initialize' of https://github.com/pablopen/k…
Jun 19, 2018
cb23c27
Merge branch 'master' into fix_client_initialize
Jun 19, 2018
7a1971e
Added test for reinitializing pool after destroy
elhigu Jul 2, 2018
52ddad9
Fixed destroy / reinitialization test
elhigu Jul 2, 2018
3997b75
Fixed the fix
elhigu Jul 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ assign(Client.prototype, {
})
},

initializePool(config) {
initializePool(config = this.config) {
if (this.pool) {
this.logger.warn('The pool has already been initialized')
return
Expand Down
2 changes: 1 addition & 1 deletion src/util/make-knex.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export default function makeKnex(client) {

// Typically never needed, initializes the pool for a knex client.
initialize(config) {
return client.initialize(config)
return client.initializePool(config)
},

// Convenience method for tearing down the pool.
Expand Down
13 changes: 13 additions & 0 deletions test/integration/suite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,17 @@ module.exports = function(knex) {
});
});

describe('knex.initialize', function() {
it('should allow initialize the pool with knex.initialize', function() {
expect(knex.client.pool).to.equal(undefined);
knex.initialize();
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this missing pool destroy before trying to reinitialize? Also this is not actually waiting that initialization is done before testing that knex was initialized.

Also check that before calling init and after calling destroy pool is actually destroyed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, at that time of the test the pool is currently destroyed. I guess that you want the test to check first if the pool is initialized (maybe with !client.pool), and if not destroy it. The waiting until is destroyed is what i don't really know how to implement

Copy link
Member

Choose a reason for hiding this comment

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

Looks like initialization is indeed synchronous opeartion, so that actually doesn't have to be waited. Yep, checking client.pool to not exist should be enough for testing that knex was not initialized at start of the test.

expect(knex.client.pool.destroyed).to.equal(false);
return knex.destroy().then(() => {
expect(knex.client.pool.destroyed).to.equal(true);
knex.init();
expect(knex.client.pool.destroyed).to.equal(false);
})
});
});

};