Skip to content

Conversation

alsotang
Copy link
Contributor

Refactor PoolNamespace#_getClusterNode but still backward-compatible

original issue: #1505

@alsotang
Copy link
Contributor Author

alsotang commented Aug 26, 2016

now we can use cluster this way:

var pool = cluster.of('SLAVE*', 'ORDER');

pool.query('SELECT 1', function (err, rows) {}) // works fine!!

@alsotang
Copy link
Contributor Author

travis ci fails, but seems a old test result in that.

@dougwilson dougwilson self-assigned this Aug 26, 2016
@dougwilson
Copy link
Member

Nice, thanks, @alsotang! Don't worry about that failing test; I'll get it fixed today :)

@dougwilson
Copy link
Member

Sorry for the long delay, @alsotang ! I was able to fix the flaky tests, and so the PR is now passing the tests. I do see that the PR is marked failing as well because it caused the coverage percentage to decrease. Looking at the report, it looks like it's because in the new PoolNamespace.prototype.query, both of the if (err) code paths are not tested.

Copy link
Member

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Pending tests for new code paths. The only missing ones are displayed at https://coveralls.io/builds/7620472/source?filename=lib%2FPoolNamespace.js

@dougwilson
Copy link
Member

lol @alsotang I just popped into this PR to see if I could just add the tests & merge to help out :) I see you just put more more commits. Please ping me when you're ready for me to take a look :)

@alsotang
Copy link
Contributor Author

I already add tests for the error handler. Seems now the PoolNamespace.js is 100% cover.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants