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: Add test for sendall min-fee setting #26622

Merged
merged 1 commit into from
Dec 3, 2022
Merged

test: Add test for sendall min-fee setting #26622

merged 1 commit into from
Dec 3, 2022

Conversation

aureleoules
Copy link
Member

While experimenting with mutation testing it appeared that the minimum fee-rate check was not tested for the sendall RPC.

https://bcm-ui.aureleoules.com/mutations/3581479318544ea6b97f788cec6e6ef1

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 1, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v, 0xB10C, ishaanam

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@maflcko maflcko changed the title test(sendall): Add test for min-fee setting test: Add test for sendall min-fee setting Dec 1, 2022
@DrahtBot DrahtBot added the Tests label Dec 1, 2022
stickies-v
stickies-v reviewed Dec 1, 2022
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

Concept ACK, but any reason not to put this in wallet_sendall.py?

@aureleoules
Copy link
Member Author

Concept ACK, but any reason not to put this in wallet_sendall.py?

You're right it makes more sense. Updated.

stickies-v
stickies-v reviewed Dec 2, 2022
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

Approach ACK 705a03c

test/functional/wallet_sendall.py Outdated Show resolved Hide resolved
test/functional/wallet_sendall.py Show resolved Hide resolved
test/functional/wallet_sendall.py Outdated Show resolved Hide resolved
@aureleoules
Copy link
Member Author

Updated per feedback, thanks @stickies-v.

stickies-v
stickies-v approved these changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK 1bfa0de

test/functional/wallet_sendall.py Outdated Show resolved Hide resolved
test/functional/wallet_sendall.py Outdated Show resolved Hide resolved
stickies-v
stickies-v approved these changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

re-ACK cb44c59

@0xB10C
Copy link
Contributor

0xB10C commented Dec 2, 2022

ACK cb44c59

Applied the diff from your mutation test and checked that the test now fails and would kill the mutation.

Maybe a feature to consider for your mutation testing: Pushing a failed mutation diff to a branch somewhere. Allows to quickly cherry-pick the fix on top and test that it's fixed.

@aureleoules
Copy link
Member Author

Maybe a feature to consider for your mutation testing: Pushing a failed mutation diff to a branch somewhere. Allows to quickly cherry-pick the fix on top and test that it's fixed.

Thanks, will look into it.

@ishaanam
Copy link
Contributor

ishaanam commented Dec 2, 2022

ACK cb44c59

@maflcko maflcko merged commit cac29f5 into bitcoin:master Dec 3, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 4, 2022
Merge bitcoin#26622: test: Add test for sendall min-fee setting
5ec826c 
cb44c59 test: Add sendall test for min-fee setting (Aurèle Oulès)

Pull request description:

  While experimenting with mutation testing it appeared that the minimum fee-rate check was not tested for the `sendall` RPC.

  https://bcm-ui.aureleoules.com/mutations/3581479318544ea6b97f788cec6e6ef1

ACKs for top commit:
  0xB10C:
    ACK cb44c59
  ishaanam:
    ACK cb44c59
  stickies-v:
    re-ACK [cb44c59](bitcoin@cb44c59)

Tree-SHA512: 31978436e1f01cc6abf44addc62b6887e65611e9a7ae7dc72e6a73cdfdb3a6a4f0a6c53043b47ecd1b10fc902385a172921e68818a7f5061c96e5e1ef5280b48
@aureleoules aureleoules deleted the 2022-11-test-sendall-min-fee branch January 12, 2023 11:51
@bitcoin bitcoin locked and limited conversation to collaborators Jan 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stickies-v stickies-v stickies-v approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@aureleoules @DrahtBot @0xB10C @ishaanam @stickies-v @maflcko