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

[Test]: Jest to Vitest migration caravan-psbt package #133

Open
wants to merge 16 commits into
base: vite-migration
Choose a base branch
from

Conversation

DhairyaMajmudar
Copy link

@DhairyaMajmudar DhairyaMajmudar commented Sep 12, 2024

Jest to Vitest migration caravan-psbt package

Related to #115

Screenshot

image

cc: @bucko13 @Legend101Zz

Copy link

changeset-bot bot commented Sep 12, 2024

⚠️ No Changeset found

Latest commit: 54379a8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Sep 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
caravan-coordinator ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 27, 2024 10:16pm

@DhairyaMajmudar
Copy link
Author

DhairyaMajmudar commented Sep 12, 2024

Out of 4 test 3, tests are passing but 1 test from psbt.test.ts file is failing below are the error logs

Screenshot from 2024-09-12 22-37-53

Error Code text
src/__tests__/psbtv0/psbt.test.ts [ src/__tests__/psbtv0/psbt.test.ts ]
Error: ecc library invalid
   assert ../../node_modules/bitcoinjs-lib-v6/src/ecc_lib.js:70:20
    verifyEcc ../../node_modules/bitcoinjs-lib-v6/src/ecc_lib.js:27:3
    Proxy.initEccLib ../../node_modules/bitcoinjs-lib-v6/src/ecc_lib.js:11:5
    src/psbtv0/psbt.ts:27:9
     25| import { autoLoadPSBT } from "./utils";
     26| 
     27| bitcoin.initEccLib(ecc);
       |         ^
     28| 
     29| export interface PsbtInput {
   src/__tests__/psbtv0/psbt.test.ts:2:31```
</details>

@DhairyaMajmudar
Copy link
Author

While finding the fix for this I found a similar kind of issue here bitcoinjs/tiny-secp256k1#136
@bucko13 can you pls. help in this

@bucko13
Copy link
Contributor

bucko13 commented Sep 12, 2024

Out of 4 test 3, tests are passing but 1 test from psbt.test.ts file is failing below are the error logs

Screenshot from 2024-09-12 22-37-53

Error Code text

I think I documented some of this including changes I had to make for jest in the readme of the package itself. See if anything there helps, otherwise I can take a further look.

@DhairyaMajmudar
Copy link
Author

Thank you @bucko13, I've found the fix from the readme
Now all tests are passing : )

Copy link
Contributor

@bucko13 bucko13 left a comment

Choose a reason for hiding this comment

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

Mostly looks good with some good dependency and configuration cleanup!

I have a question about the globals and if we can avoid the need for the test tooling imports and also adding some documentation for how to handle different js runtime environments.

packages/caravan-psbt/package.json Show resolved Hide resolved
packages/caravan-psbt/src/__tests__/functions.test.ts Outdated Show resolved Hide resolved
"eslint-plugin-prettier": "^5.1.3",
"jest": "^29.4.1",
"jsdom": "24.0.0",
"jsdom-global": "3.0.2",
"lodash": "^4.17.21",
"react-silence": "^1.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you replaced the uses of this right? so can probably uninstall.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I haven't uninstalled them completely since those modules are shared in other packages, if uninstalled the test suite there will fail.

I'll remove those unwanted modules once every package gets migrated to vitest : )

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. That shouldn’t be the case with the monorepo since they should be isolated. Any package that uses this should have it in its own package.json

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, every package has its own package.json but the node_modules and package-lock.json files are shared among them

Copy link
Contributor

Choose a reason for hiding this comment

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

Right so you should be able to remove it from THIS package.json, re-run npm install to update the package lock accordingly and then everything else should still worn

Copy link
Author

Choose a reason for hiding this comment

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

I've re-run the npm i command and pushed the changes in package-lock.json file in my latest commit. I've also checked if the tests are running fine and they are.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right I don’t imagine anything has changed. It looks like react-silence is still in this packages package.json. I think you can remove it and then re-run everything and it should be fine. If it’s not working then that means another package needs to be explicit about the react silence dev dependency.

Copy link
Author

Choose a reason for hiding this comment

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

I've removed react-silence from package.json of psbt but after doing so the tests in caravan-bitcoin were failing so I've installed the same in its own package.json file

packages/caravan-psbt/tsconfig.json Show resolved Hide resolved
@DhairyaMajmudar
Copy link
Author

Mostly looks good with some good dependency and configuration cleanup!

That's amazing thank you @bucko13

I have a question about the globals and if we can avoid the need for the test tooling imports

Yeah, I've removed the unwanted imports from both caravan-psbt as well as caravan-client

adding some documentation for how to handle different js runtime environments.

I've modified the readme section by adding a new testing section there

"@inrupt/jest-jsdom-polyfills": "^3.2.1",
"@jest/globals": "^29.7.0",
"@types/jest": "^29.5.12",
"@vitejs/plugin-react": "^4.3.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

why was this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

I had moved that dependency to the root package.json file as of which I've removed it from here

Copy link
Contributor

@bucko13 bucko13 left a comment

Choose a reason for hiding this comment

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

Just one question about the inclusion of @vitejs/plugin-react otherwise this looks ready to go!

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