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

[DhairyaMajmudar]

Jest to Vitest migration caravan-client package

Related to #115

Screenshot

cc: @bucko13 @Legend101Zz

[changeset-bot]

⚠️ No Changeset found

Latest commit: cb512ba

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]

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]

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 : )

[bucko13]

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]

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]

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]

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]

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]

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]

Nice! Glad it's passing now. Just a couple other small things (which should be reusable for the next rounds/packages!

[bucko13]

So I started to look at some turborepo examples to reference because I was a little confused since you're not actually using the config anywhere as far as I can see (it's not extended or imported for example). And it looks like maybe it's overkill to do it in this way.

Here is an example from turborepo.

What I was thinking about though you can see in this example from turborepo for eslint where there is a base package and then it's imported and extended in sub packages as they're needed.

Perhaps that's not needed though and everything can share the same base vitest config at the root.

Sorry for the back and forth on this! Just wanting to make sure we get it right while we're in the process of refactoring.

[bucko13]

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]

Got your point here @bucko13 #119 (review) 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 #119 (comment) 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 : )