Skip to content

chore: update the document. #964

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.

Sign up for GitHub

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

Merged
krpeacock merged 2 commits into from
Jan 22, 2025
Merged

chore: update the document. #964

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
684e307
Update the document.
vincent-dfinity Jan 21, 2025
20b25b6
Merge branch 'main' into vincent/update-doc
krpeacock Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ After that, you probably want to dive into a specific package in [./packages](./

Running tests is a good way to get a sense of what the features will do. We try to have full unit test coverage for all new features, although sometimes mocking network conditions can be difficult, and e2e tests may be preferable.

Before running tests, you need to compile the packages.
Copy link
Member

@peterpeterparker peterpeterparker Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Randomly came across this PR. It feels somewhat uncommon to require building before running the tests — at least I'm not aware of any other JS libraries at the foundation that need such a step. Shouldn't the issue be fixed instead?

Just my two cents of course, feel free to ignore.

Copy link
Contributor Author

@vincent-dfinity vincent-dfinity Jan 21, 2025

Choose a reason for hiding this comment

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

Agree. I'm just working on adding a feature to agent-js, but I cannot run the tests successfully by following the steps. So I think it's better to have it fixed first.

I can discuss with Kaia to see how to address your concerns afterward.

Copy link
Contributor

@krpeacock krpeacock Jan 22, 2025

Choose a reason for hiding this comment

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

I spent a half hour comparing the ic-js and agent-js Jest configs yesterday and haven't found the culprit yet. I'll create a ticket for figuring out why jest isn't working without building the packages

Copy link
Contributor Author

@vincent-dfinity vincent-dfinity Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I tried to run the test in ic-js, and I have to do npm run build as well. Otherwise, I got the similar error as below

~/repos/ic-js$ npm test

> @dfinity/[email protected] test
> jest

 FAIL  packages/nns/src/utils/neurons.utils.spec.ts
  ● Test suite failed to run

    packages/nns/src/utils/neurons.utils.spec.ts:2:39 - error TS2307: Cannot find module '@dfinity/utils' or its corresponding type declarations.

    2 import { uint8ArrayToHexString } from "@dfinity/utils";
                                            ~~~~~~~~~~~~~~~~

 PASS  packages/utils/src/utils/date.utils.spec.ts (6.511 s)
 FAIL  packages/nns/src/canisters/governance/request.converters.spec.ts
  ● Test suite failed to run

I have to do

npm install
npm run build
npm test

to run the tests successfully.

Could you guys confirm it as well? Not sure if I did something wrong.

Copy link
Member

@peterpeterparker peterpeterparker Jan 23, 2025

Choose a reason for hiding this comment

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

Ahhhhhhh, yes, I can confirm. Now I get it. It's actually a phrasing observation.

Instead of the sentence provided in this PR:

Before running tests, you need to compile the packages.

Maybe we should write the following:

Before running tests **for the very first time**, you need to compile the packages.


```bash
npm run build
```

This command will compile the packages and generate the output under `lib` directory in each package.

#### Unit Tests

To run the unit tests for all packages, run `npm test`. You can run tests for a specific package by running `npm test` in the package directory or by running `npm test --workspace=<package-name>` in the root directory.
Expand Down
Loading