Skip to content

Conversation

@vincent-dfinity
Copy link
Contributor

@vincent-dfinity vincent-dfinity commented Jan 21, 2025

Description

Update the document about how to run the tests.

Fixes # (issue)

How Has This Been Tested?

Only document change.

Checklist:

  • My changes follow the guidelines in CONTRIBUTING.md.
  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@vincent-dfinity vincent-dfinity requested a review from a team as a code owner January 21, 2025 06:33
@github-actions
Copy link
Contributor

github-actions bot commented Jan 21, 2025

size-limit report 📦

Path Size
@dfinity/agent 72.09 KB (0%)
@dfinity/candid 11.65 KB (0%)
@dfinity/principal 4.16 KB (0%)
@dfinity/auth-client 50.09 KB (0%)
@dfinity/assets 67.85 KB (0%)
@dfinity/identity 47.74 KB (0%)
@dfinity/identity-secp256k1 106.31 KB (0%)


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

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

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

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

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

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.

@krpeacock krpeacock enabled auto-merge (squash) January 22, 2025 18:05
@krpeacock krpeacock merged commit ce618eb into main Jan 22, 2025
15 checks passed
@krpeacock krpeacock deleted the vincent/update-doc branch January 22, 2025 18:08
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.

3 participants