search

caravan-bitcoin / caravan

Caravan monorepo
https://caravanmultisig.com
MIT License
37 stars 32 forks source link

[Test]: Jest to Vitest migration `caravan-client` package #119

Closed DhairyaMajmudar closed 2 months ago

DhairyaMajmudar commented 3 months ago

Jest to Vitest migration caravan-client package

Related to #115

Screenshot

image

cc: @bucko13 @Legend101Zz

changeset-bot[bot] commented 3 months ago

⚠️ No Changeset found

Latest commit: cb512babb4e76e31b1c46b357d14944a8ebd9efd

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

vercel[bot] commented 3 months ago

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 Aug 30, 2024 6:40am
DhairyaMajmudar commented 3 months ago

Needed one suggestion:

How's about keeping test files in __test__ folder this will help test and main files to keep separated and also gives below looking cool folder structure : )

image

bucko13 commented 3 months ago

Very nice! Reorganizing to a tests or test directory sounds good to me as well.

Something also to consider as part of the larger #115 issue is to update the template generator and tooling packages to have and use a shared vitest config as well as update the shared tsconfig to include the vitest

bucko13 commented 3 months ago

CI seems to be having trouble with this for some reason: https://github.com/caravan-bitcoin/caravan/actions/runs/10584531073/job/29340663941?pr=119

Have you tried running npm run ci locally to see if it's reproducible? I can't see anything obvious in the diff.

DhairyaMajmudar commented 3 months ago

Very nice! Reorganizing to a tests or test directory sounds good to me as well.

Something also to consider as part of the larger #115 issue is to update the template generator and tooling packages to have and use a shared vitest config as well as update the shared tsconfig to include the vitest

Thank you! yeah keeping a common vitest config file will be a great idea leveraging the power of monorepos : )

DhairyaMajmudar commented 3 months ago

CI seems to be having trouble with this for some reason: https://github.com/caravan-bitcoin/caravan/actions/runs/10584531073/job/29340663941?pr=119

Have you tried running npm run ci locally to see if it's reproducible? I can't see anything obvious in the diff.

I've run the command in local and while running so I've observed that the process is not getting exit after completion, this might be the reason Github actions got cancelled after a certain time

I think it will be good if we separate the linting, testing and building pipelines, and that too in both package and app folders (If files are changed and package folder/s than CI will run only for that not for app folder/s) this will reduce the GHA execution time.

What are your thoughts on this @bucko13 ?

bucko13 commented 3 months ago

this might be the reason Github actions got cancelled after a certain time

it actually never canceled! I had to manually run it.

I think it will be good if we separate the linting, testing and building pipelines,

You can try that for sure, but just want to note that the CI is passing in other PRs. It's just this one where it gets stuck.

DhairyaMajmudar commented 3 months ago

this might be the reason Github actions got cancelled after a certain time

it actually never canceled! I had to manually run it.

I think it will be good if we separate the linting, testing and building pipelines,

You can try that for sure, but just want to note that the CI is passing in other PRs. It's just this one where it gets stuck.

okayyy! I've made minor changes in the test scripts and it seems like now ci is passing.

bucko13 commented 3 months ago

Also, because you applied the import order in the base eslint, then all other packages inherit that rule, and now the lint step is failing. Either we should do those one at a time in the individual child configs and then can move that to the parent when they're all done, or don't add that rule yet and do that in a separate PR separate from the testing changes.

I do like that rule and would like to add it, but I think it increases the scope of these efforts to upgrade testing (many more files in the diff!)

DhairyaMajmudar commented 2 months ago

Got your point here @bucko13 https://github.com/caravan-bitcoin/caravan/pull/119#pullrequestreview-2281759260 let me revert back the changes from the last commit and get back the vitest config file in the root itself.

As of the comment here https://github.com/caravan-bitcoin/caravan/pull/119#issuecomment-2330512515 yeah, it will be a good decision to open a separate PR for correcting the import orders from the codebase (Let me open one once this PR is merged)

EDIT: I've reverted back the last commit. If it looks good let's merge : )