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.

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

cli: add simple integration tests #937

Closed
wants to merge 5 commits into from

Conversation

alxndrsn
Copy link
Contributor

@alxndrsn alxndrsn commented Jul 27, 2023

I wrote these tests for confidence before merging #892.

@alxndrsn alxndrsn force-pushed the cli-integration-tests branch 3 times, most recently from a760276 to b05833e Compare July 27, 2023 08:39
@alxndrsn alxndrsn force-pushed the cli-integration-tests branch from b05833e to 3432f3d Compare September 6, 2023 08:30
@alxndrsn
Copy link
Contributor Author

alxndrsn commented Sep 6, 2023

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
Copy link
Member

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.

});

describe('command: user-create', () => {
it('should return status code 0 and user details on success', () => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@alxndrsn alxndrsn Sep 13, 2023

Choose a reason for hiding this comment

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

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(?)

Copy link
Member

@matthew-white matthew-white Sep 13, 2023

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

alxndrsn commented Sep 13, 2023

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
Copy link
Member

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
Copy link
Contributor Author

alxndrsn commented Sep 16, 2023

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
Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

2 participants