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

clients/js: add unit tests on worm CLI #2631

Closed

Conversation

AlberErre
Copy link
Contributor

@AlberErre AlberErre commented Apr 13, 2023

Resolves #2596

This PR adds unit tests for cmds commands on worm CLI 🔒🚓 located on client/ts/__tests__

In order to run them, you need to navigate to client/js folder and run:

npm run test

Also, a jest.config.json file has been added to configure jest testing environment.


🚨 Note: In order to test a command module, it needs to export all the module properties, following this documentation.

There are some missing tests modules on this PR, such as evm, aptos, generate. These modules need refactor to be able to test them, as they don't expose all the required interfaces, in particular exports.handler.

This refactor should be implemented in the future, as it's beyond the scope of this PR.

✅ Update:

All the worm commands are now covered with a test suite, checking all commands with their expected positional arguments, as well as checking if these commands are in sync with the auto-generated documentation (README.md).
In order to achieve this, we compare the worm CLI output expected by worm <command>--help pattern 👍🏻

Also, I've added functionality tests (to cover all cases dynamically) for the following commands:

  • worm parse
  • worm recover
  • worm info chain-id
  • worm info rpc
  • worm info contract

Once all these tests are executed, a HTML report is auto-generated to evaluate them if necessary 📝. This report is generated for every npm run test execution and is stored on client/js/html-report/worm-cli-tests-report.html.

Finally, a Github Actions (.github/workflows/worm-cli.yml) has been created to integrate all these tests & build process into wormhole pipelines 🚀

Here is a recent Github Action pipeline I run successfully on my fork ✅:
https://github.com/AlberErre/wormhole/actions/runs/5337687871

@AlberErre
Copy link
Contributor Author

@heyitaki @kcsongor @aadam-10 @evan-gray could you take a look at this? 👀

feedback and suggestions are more than welcome! 🙏🏼 🚀

@aadam-10
Copy link
Contributor

hi @AlberErre, we're currently dealing with other priority items but we'll definitely take a look at this soon.

clients/js/__tests__/utils/getters.ts Outdated Show resolved Hide resolved
clients/js/__tests__/utils/getters.ts Outdated Show resolved Hide resolved
clients/js/__tests__/recover.test.ts Show resolved Hide resolved
"copy-dir": "^1.3.0",
"jest": "^29.5.0",
"jest-html-reporters": "^3.1.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of generating an HTML report? Is there an advantage to running tests and viewing results in the CLI? Would rather keep dependency list as slim as possible!

Copy link
Contributor Author

@AlberErre AlberErre Jul 13, 2023

Choose a reason for hiding this comment

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

This is useful while tests are failing in the pipeline 🚨

By using this HTML report we can see which tests are failing (and why) more easily 🏄‍♂️. The current alternative is scrolling through CLI (Jest) error output, which is harder to read and reason about.

Regarding slimming our dependency list, this package is included in devDependencies 👀 . Therefore this dependency won't be included in the Worm CLI build/main.js executable output, and won't increase bundle size.

This dependency would only live during the lifespan of the Github Action, similar to what Jest dependency does.

Copy link
Contributor

@heyitaki heyitaki Jul 15, 2023

Choose a reason for hiding this comment

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

Hmm CLI output is usually enough for me, but I'm fine with it if others are I guess 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heyitaki just removed the HTML report generation on the next PR 👍 #3229

clients/js/src/cmds/index.ts Outdated Show resolved Hide resolved
clients/js/src/main.ts Outdated Show resolved Hide resolved
@AlberErre AlberErre requested a review from heyitaki July 13, 2023 20:10
@AlberErre
Copy link
Contributor Author

@evan-gray could you take a look at this MR? 🙏 The changes you requested have been applied ✅

@AlberErre
Copy link
Contributor Author

An extension of this PR (covering worm submit command) has been created here: #3229

@evan-gray
Copy link
Contributor

Closing this as some of these tests do not appear to be sufficiently meaningful or are already covered by other tests of the CLI. For example, the README.md is more efficiently covered by the check added in #3058

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.

clients/js: add tests
4 participants