Skip to content
This repository has been archived by the owner. It is now read-only.

chore(deps): Update mysql package dependency to latest version. #112

Merged
merged 2 commits into from Dec 16, 2015

Conversation

@rfk
Copy link
Member

@rfk rfk commented Oct 30, 2015

Because AFAICT it's got some changes to how pools are handled, and we want to eliminate variables while digging into #108.

@rfk rfk added this to the FxA-0: quality milestone Oct 30, 2015
@rfk rfk self-assigned this Oct 30, 2015
@vladikoff
Copy link
Contributor

@vladikoff vladikoff commented Nov 2, 2015

@jrgm and @rfk to check this

@rfk
Copy link
Member Author

@rfk rfk commented Nov 16, 2015

Per discussion with @jrgm, we'll push ahead with trying to make this update stick and then try it out to see if it resolves the connection problem in #108. Unfortunately I'm starting to suspect some sort of bug in the mysql package, as the test failure here is due to a timeout when calling connection.destroy(). AFAICT we're calling it properly so it really shouldn't cause any times, I need to dig into it more.

@rfk rfk force-pushed the rfk/update-mysql-version branch from b2f75a4 to 93e9e00 Nov 18, 2015
@rfk
Copy link
Member Author

@rfk rfk commented Nov 26, 2015

Blocked on upstream bug: mysqljs/mysql#1291

@rfk rfk force-pushed the rfk/update-mysql-version branch 2 times, most recently from a69d2b2 to c6e5773 Nov 26, 2015
@rfk
Copy link
Member Author

@rfk rfk commented Nov 26, 2015

Well, this escalated quickly...

The intended upgrade to latest node-mysql is indeed blocked by an upstream bug, but in order to find and fix that bug, I ended up spending a lot of quality time with the test suite. I'm rolling my testsuite changes into this PR as well, which include:

  • Upgrade to latest tap runner, which includes much better debugging information on timeouts and is what ultimately led to me tracking down the bug
  • Latest tap also handles promises well by default, so remove our ptaptest wrapper as well as several no-longer-necessary calls to t.end().
  • A bunch of our t.plan calls were stating the wrong number of tests, and these started failing in the newer test runner
  • When you use t.plan(n), the test runner will start running the next test as soon as your n'th assertion passes, so if you've got cleanup code to run at the end of a test, racy things can happen. Some tests have grown an additional "and all was well" check at the end of them in order to prevent this.

With mysqljs/mysql#1291 applied locally, I get a clean (and significantly nicer-looking) test run. whew

@rfk rfk force-pushed the rfk/update-mysql-version branch from c6e5773 to 3280268 Dec 15, 2015
@rfk rfk assigned philbooth and unassigned rfk Dec 15, 2015
@rfk
Copy link
Member Author

@rfk rfk commented Dec 15, 2015

Alrighty, this is finally ready for review!

It turned out quite churny due to the test updates, but they're mostly mechanical e.g. updating t.plan constants, returning promises rather than calling t.end. @philbooth r?

@@ -169,10 +169,6 @@ module.exports = function (log, error) {
Memory.prototype.createAccountResetToken = function (tokenId, accountResetToken) {
tokenId = tokenId.toString('hex')

if ( accountResetTokens[tokenId] ) {

This comment has been minimized.

@philbooth

philbooth Dec 16, 2015
Contributor

The comments don't mention the reason for this but presumably it was just some errant rejection that wasn't being picked up by the pre-promisified tests? Is there a case to be made for an explicit assertion somewhere to cover it, or is it sufficient that the promise simply doesn't reject?

This comment has been minimized.

@rfk

rfk Dec 16, 2015
Author Member

This behaviour didn't match the mysql backend, which silently replaces any existing reset token with a new one. When I upgraded the test runner, the tests started failing with this error.duplicate. So AFAICT we already have test coverage for, for some reason the test was just failing to fail with the old version of the runner!

@philbooth
Copy link
Contributor

@philbooth philbooth commented Dec 16, 2015

Looks good to me. 👍

rfk added a commit that referenced this pull request Dec 16, 2015
chore(deps): Update mysql package dependency to latest version.
@rfk rfk merged commit b5a38f8 into master Dec 16, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@rfk rfk removed the waffle:review label Dec 16, 2015
@shane-tomlinson shane-tomlinson deleted the rfk/update-mysql-version branch Apr 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants