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.

Sign up for GitHub

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

Jump to bottom

test: fix all broken validations in packages folder #3495

Closed
wants to merge 2 commits into from
Closed

test: fix all broken validations in packages folder #3495

wants to merge 2 commits into from

Conversation

ashnashahgrover
Copy link
Contributor

@ashnashahgrover ashnashahgrover commented Aug 27, 2024 •
edited
Loading

Commit to be reviewed

test: fix all broken validations in packages folder 


Primary Changes
---------------
1. Fixed broken assertions in deploy-contract-from-json-xdai.test.ts so they assert that 
“Nonce is too low” as expected.

Fixes #3493

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.

petermetz
petermetz requested changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@ashnashahgrover Please fix the commit lint check and make sure to squash the commits into one and rebase onto upstream main

@RafaelAPB
Copy link
Contributor

Thank you for the PR. Please fix the lint problems and rebase, as @petermetz mentioned.
Could you please elaborate on the goal of the PR? The description states 1. Fixed broken assertions in deploy-contract-from-json-xdai.test.ts so they assert that “Nonce is too low, which makes sense, but there are 14 files changed.

@ashnashahgrover ashnashahgrover force-pushed the ashnashahgrover/issue3493 branch 3 times, most recently from 4d717a0 to 0053b3e Compare September 3, 2024 00:19
@ashnashahgrover
Copy link
Contributor Author

Thank you for the PR. Please fix the lint problems and rebase, as @petermetz mentioned. Could you please elaborate on the goal of the PR? The description states 1. Fixed broken assertions in deploy-contract-from-json-xdai.test.ts so they assert that “Nonce is too low, which makes sense, but there are 14 files changed.

I fixed the commit issue. The 14 files changed comes from a mistake I might have made in my understanding.... Since this issue is a dependency of issue 3483, I thought I needed to branch it off issue 3483's branch. So it was branched off that and not main.

Should I just make a new PR with the same changes at this point?

@petermetz
Copy link
Contributor

Thank you for the PR. Please fix the lint problems and rebase, as @petermetz mentioned. Could you please elaborate on the goal of the PR? The description states 1. Fixed broken assertions in deploy-contract-from-json-xdai.test.ts so they assert that “Nonce is too low, which makes sense, but there are 14 files changed.

I fixed the commit issue. The 14 files changed comes from a mistake I might have made in my understanding.... Since this issue is a dependency of issue 3483, I thought I needed to branch it off issue 3483's branch. So it was branched off that and not main.

Should I just make a new PR with the same changes at this point?

@ashnashahgrover No worries! If this is a dependency of another one then we can just mark it as such and then we'll return to the review once the parent PR (the one this depends on) has been merged. Then we can consolidate the diff down to the actual changes that are going into this one and it will all be nice and tidy and easy to review.

No need to start a new PR, more than that, it's explicitly against the contribution guidelines to do so because then the review trail we've established is reset and a lot of duplicate work has to be done to review the new PR again in that case, so no new PRs needed.

Please see my latest post on discord about how to declare the PR dependency: https://discord.com/channels/905194001349627914/908379338716631050/1281022175620366450

Peter Somogyvari — Today at 3:44 PM
Everyone: We are testing a new PR dependency declaration robot. It works pretty much the same way as the old one did but it looks like it's more permissible with the syntax depends on #??? can be placed anywhere in the PR description within any sentence from what I can tell from their documention (which is this friendly looking gif => https://marketplace-screenshots.githubusercontent.com/7171/4746c300-c629-11ea-85b9-44c0c5ecfe83 )

I've enabled it as a required check on the upstream repo right now so in theory it will just work but please keep me in the loop if you see any problems with any of your pull requests related to the new robot!

Cheers!

@petermetz
Copy link
Contributor

@ashnashahgrover Once the parent PR is merged you can just hit the re-request review button and this will pop back up on my radar for a final review (make sure to rebase onto upstream/main right before you request the new review)

@ashnashahgrover
test: refactor jest test negative test cases - phase 1
f2c27ea 
Primary Changes
---------------
1. Refactored all remaining negative test case exception assertions under cacti/packages
Removed try-catch blocks, replaced with declarations through jest-extended's own API.

Fixes hyperledger-cacti#3483

Signed-off-by: ashnashahgrover <ashnashahgrover777@gmail.com>
@ashnashahgrover
test: fix all broken validations in packages folder 
1e35f55 
Primary Changes
---------------
1. Fixed broken assertions in deploy-contract-from-json-xdai.test.ts so they assert that
“Nonce is too low” as expected.

Fixes hyperledger-cacti#3493

Signed-off-by: ashnashahgrover <ashnashahgrover777@gmail.com>
@petermetz
Copy link
Contributor

Closing this due to inactivity, please feel free to re-open anytime if/when bandwidth is available to take it to the finish line.

@petermetz petermetz closed this Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petermetz petermetz petermetz requested changes

@takeutak takeutak Awaiting requested review from takeutak

@izuru0 izuru0 Awaiting requested review from izuru0 izuru0 is a code owner

@jagpreetsinghsasan jagpreetsinghsasan Awaiting requested review from jagpreetsinghsasan jagpreetsinghsasan is a code owner

@VRamakrishna VRamakrishna Awaiting requested review from VRamakrishna VRamakrishna is a code owner

@sandeepnRES sandeepnRES Awaiting requested review from sandeepnRES sandeepnRES is a code owner

@outSH outSH Awaiting requested review from outSH outSH is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test: fix all broken validations in packages folder
3 participants
@ashnashahgrover @RafaelAPB @petermetz