Skip to content

test(weaver/interoperation-node-sdk): fix broken nyc tests #3935

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hollermay
Copy link

@hollermay hollermay commented Jun 1, 2025

This pull request resolves #3305
Adds [email protected] as a direct dependency to the Fabric interoperation node SDK to resolve ESM compatibility issues that were causing test failures. This ensures proper module resolution when running tests and maintains compatibility with the SDK's existing dependencies.

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

@hollermay
Copy link
Author

@takeutak @petermetz kindly review my PR

@sandeepnRES
Copy link
Contributor

sandeepnRES commented Jun 3, 2025

Also @hollermay can you make the PR and the commit title same as issue title. That's a guideline in Cacti. This makes it easy to track PRs and its corresponding issues.
The description to the solution you already have it in the commit message body.
Thank you.

@hollermay hollermay changed the title fix(weaver): add get-func-name dependency to fabric interop SDK test(weaver/interoperation-node-sdk): fix broken nyc tests Jun 3, 2025
@hollermay
Copy link
Author

Also @hollermay can you make the PR and the commit title same as issue title. That's a guideline in Cacti. This makes it easy to track PRs and its corresponding issues.
The description to the solution you already have it in the commit message body.
Thank you.

I have made the changes to the title. Kindly check.

@sandeepnRES
Copy link
Contributor

Also @hollermay can you make the PR and the commit title same as issue title. That's a guideline in Cacti. This makes it easy to track PRs and its corresponding issues.
The description to the solution you already have it in the commit message body.
Thank you.

I have made the changes to the title. Kindly check.

Thanks, please make the commit title same too.

@sandeepnRES
Copy link
Contributor

@hollermay You will need to sign the commit, that's why DCO check is failing.
Either use git commit -s when you commit, in case you have already committed and want to sign just the last commit use:
git commit --amend --signoff

Copy link
Contributor

@sandeepnRES sandeepnRES left a comment

Choose a reason for hiding this comment

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

Looks good to me, all tests are passing hence approving.
Please address the comments above before merging.

@hollermay hollermay changed the title test(weaver/interoperation-node-sdk): fix broken nyc tests test(weaver/interoperation-node-sdk): fix broken nyc tests Jun 4, 2025
@hollermay
Copy link
Author

Looks good to me, all tests are passing hence approving. Please address the comments above before merging.

I've addressed all the comments and did the changes as per your requirements. With the valid signoffs the PR is ready to be merged.

Copy link
Contributor

@outSH outSH left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@hollermay
Copy link
Author

@sandeepnRES any updates?

Copy link
Contributor

@sandeepnRES sandeepnRES left a comment

Choose a reason for hiding this comment

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

@hollermay It seems you created a merge commit in this PR, when updating with main.
Use only rebase and merge/update whenever updating your branch.
Can you fix this? make sure no merge commits are there, and no commits from others should show. The PR should have only one commit which is yours.
Let me know if you need help regarding this.
One way is to delete the merge commit, push the changes, and then using this github interface use update with rebase option that you can see in this PR window below.

@hollermay
Copy link
Author

@hollermay It seems you created a merge commit in this PR, when updating with main.
Use only rebase and merge/update whenever updating your branch.
Can you fix this? make sure no merge commits are there, and no commits from others should show. The PR should have only one commit which is yours.
Let me know if you need help regarding this.
One way is to delete the merge commit, push the changes, and then using this github interface use update with rebase option that you can see in this PR window below.

On it!

Adds [email protected] as a direct dependency to the Fabric interoperation
node SDK to resolve ESM compatibility issues that were causing test failures.
This ensures proper module resolution when running tests and maintains
compatibility with the SDK's existing dependencies.

Signed-off-by: hollermay <[email protected]>
@hollermay
Copy link
Author

@sandeepnRES I have done the changes as required.

@hollermay
Copy link
Author

@sandeepnRES kindly check

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.

test(weaver/interoperation-node-sdk): fix broken nyc tests
3 participants