8000 cmd: use getParams() to fix the test failure at cmd/utils/nodecmd by yoomee1313 · Pull Request #1826 · klaytn/klaytn · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Aug 19, 2024. It is now read-only.

cmd: use getParams() to fix the test failure at cmd/utils/nodecmd #1826

Merged
merged 3 commits into from
Apr 28, 2023
Merged

cmd: use getParams() to fix the test failure at cmd/utils/nodecmd #1826

merged 3 commits into from
Apr 28, 2023

Conversation

yoomee1313
Copy link
Contributor
@yoomee1313 yoomee1313 commented Apr 13, 2023 •
edited
Loading

Proposed changes

Some tests are failed due to the change at governance namespace.

  • governance.chainConfig -> governance.getParams()

I think this was not detected because the test is not executed when we create the PR.

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

  • Please leave the issue numbers or links related to this PR here.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@yoomee1313 yoomee1313 marked this pull request as ready for review April 13, 2023 09:17
@yoomee1313 yoomee1313 self-assigned this Apr 13, 2023
ian0371
ian0371 reviewed Apr 13, 2023
View reviewed changes
Copy link
Contributor
@ian0371 ian0371 left a comment

Choose a reason for hiding this comment

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

Please use governance_getParams() instead of governance_chainConfigAt().
The latter will be deprecated in v1.11 (see #1783)

ian0371
ian0371 reviewed Apr 14, 2023
View reviewed changes
@yoomee1313 yoomee1313 changed the title cmd: use chainConfigAt() to fix the test failure at cmd/utils/nodecmd cmd: use getChainConfig() to fix the test failure at cmd/utils/nodecmd Apr 14, 2023
ian0371
ian0371 previously approved these changes Apr 14, 2023
View reviewed changes
aidan-kwon
aidan-kwon previously approved these changes Apr 24, 2023
View reviewed changes
Copy link
Member
@aidan-kwon aidan-kwon left a comment

Choose a reason for hiding this comment

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

Will you add more commits to use governance_getParams()?

@yoomee1313 yoomee1313 dismissed stale reviews from aidan-kwon and ian0371 via c54110b April 24, 2023 07:33
@yoomee1313 yoomee1313 changed the title cmd: use getChainConfig() to fix the test failure at cmd/utils/nodecmd cmd: use getParams() to fix the test failure at cmd/utils/nodecmd Apr 24, 2023
@yoomee1313
Copy link
Contributor Author

@ian0371 I have revised the code to use getParams. It would be helpful if you could give a final review.

@yoomee1313 yoomee1313 merged commit 6da6872 into klaytn:dev Apr 28, 2023
@yoomee1313 yoomee1313 deleted the cmd-test-fix branch April 28, 2023 09:18
@JayChoi1736 JayChoi1736 mentioned this pull request Jul 24, 2023
Closed
20 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ian0371 ian0371 ian0371 approved these changes

@aidan-kwon aidan-kwon aidan-kwon approved these changes

@kjeom kjeom Awaiting requested review from kjeom

@kjhman21 kjhman21 Awaiting requested review from kjhman21

Assignees

@yoomee1313 yoomee1313

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yoomee1313 @ian0371 @aidan-kwon
0