cli: add simple integration tests

[alxndrsn]

I wrote these tests for confidence before merging #892.

[alxndrsn]

Failing in CI due to:

  2066 passing (8m)
  17 pending
  1 failing

  1) cli
       command: user-create
         should return status code 0 and user details on success:
     Command failed: node lib/bin/cli.js --email d4d1be75-2c6f-4dec-a948-f82cd22dccff@example.com user-create
error: relation "actees" does not exist
    at Parser.parseErrorMessage (/home/circleci/repo/node_modules/pg-protocol/dist/parser.js:287:98)
    at Parser.handlePacket (/home/circleci/repo/node_modules/pg-protocol/dist/parser.js:126:29)
    at Parser.parse (/home/circleci/repo/node_modules/pg-protocol/dist/parser.js:39:38)
    at Socket.<anonymous> (/home/circleci/repo/node_modules/pg-protocol/dist/index.js:11:42)

... it looks like the DB is not initialised correctly. Perhaps the tests need to run in one of the wrappers from test/integration/setup.js? testContainer() did not seem to help.

[matthew-white]

I think normally, tasks are tested using testTask(). But I see that you're running the actual CLI in these tests. In that case, I'm guessing the CLI will try to use the default database rather than the test one. test/integration/setup.js only initializes the test database.

[matthew-white]

It feels unclear to me what should be tested in this file vs. test/integration/task/account.js. It makes sense that this file is testing status codes. For testing output, my instinct is that that should be done in test/integration/task/account.js along with the other tests related to the behavior of each task.

[alxndrsn]

This is an integration test for lib/bin/cli.js. I'd be surprised to find it in test/integration/task/account.js. Perhaps cli.js is named misleading though, as it only deals with accounts(?)

[matthew-white]

I think that's right that cli.js only deals with accounts. It may have been named the way it was with the thought that it would eventually do more things. In the user docs, we mention an odk-cmd, which just calls cli.js I think. As far as I know, the only other script that users run directly is restore.js for restoring a backup (user docs).

[alxndrsn]

I think normally, tasks are tested using testTask(). But I see that you're running the actual CLI in these tests. In that case, I'm guessing the CLI will try to use the default database rather than the test one. test/integration/setup.js only initializes the test database.

Good spot 😬 I think using the config module's NODE_CONFIG_ENV would be one way to make this work. It would also mean we could structure the config JSON files more naturally.

[matthew-white]

I agree that the structure of the config files could probably be improved. I'd be a little hesitant to make a big change there though. Doing so would require us to update the central repo in addition to this one. Maybe something for us all to discuss in Slack?

I think the strategy up to this point has been to keep the files in lib/bin simple and put most logic in lib/task. Logic in lib/task can be tested using testTask(). However, I can see why it'd be useful to test the files in lib/bin and how the current strategy doesn't account for that.

[alxndrsn]

Doing so would require us to update the central repo in addition to this one.

And would require all deployments to change their config structure.

We could still change the test config structure, and how we load it. Example of the changes needed at #990.

[matthew-white]

Ah I see. I think it's definitely worth considering that pattern. I'll defer to @ktuite and/or @sadiqkhoja about that change, but I can see how that pattern would solve the specific problem here and how it might be an improvement overall.