knex / knex / 20025655046

93%

push

github

Fix migration CLI and cli tests (#6264)

* fix: allow capital letters in taskName construction

Also replaces disallowed strings with underscores to
make the task names more readable

* fix: actually run the CLI tests

Background: #5138 bumped Jake, and as part of the migration moved to
using child_process instead of jake.createExec. However, the events
emitted by Jake and Node when a process exits are different, so the
event handler intended to resolve the promise when the exec finished
never ran.

Since no handles held the process open, it just exited with 0, doing
nothing.

I chose to utilize the callback form of `child_process.exec` since
it already does the work these functions were doing (primarily,
accumulating the contents of stdout and stderr).

With this change, `npm test:cli` runs the tests again.

* fix: return consistently-shaped data from Migrator.js

Background: #5541 introduced lifecycle hooks into the migrator, but
avoids running them if there are no migrations to execute. It returns
an empty array from `_runBatch` in this case, but if there is work
to do it returns the result of `this._waterfallBatch`, which differs:
`_waterfallBatch` returns `[0, []]` when it has nothing to do.

The return value here is consumed in the CLI by `migrate:up`,
`migrate:rollback`, and `migrate:down`, throwing an undefined
property access error on the missing second argument.

I chose to just initialize the return value to `[0, []]` instead of
`[]` because I'm uncertain whether anything else depends on this
behavior. Plausibly, users accessing migrations via an exposed
API may have similar issues.

* Return the latest batch number, rather than 0, when running no migrations

3562 of 4206 branches covered (84.69%)

Branch coverage included in aggregate %.

2 of 2 new or added lines in 1 file covered. (100.0%)

14717 of 15433 relevant lines covered (95.36%)

2373.31 hits per line