Skip to content

test: Move wallet tests to wallet_*.py #26601

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
Merged
merged 2 commits into from
Dec 9, 2022
Merged

test: Move wallet tests to wallet_*.py #26601

merged 2 commits into from
Dec 9, 2022

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Nov 29, 2022 •
edited
Loading

The tests only tests the wallet and it doesn't make sense to extend it for other stuff, so clarify that.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 29, 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 fanquake, pablomartin4btc
Concept ACK hernanmarino

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26656 (tests: Improve runtime of some tests when --enable-debug by achow101)
  • #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot added the Tests label Nov 29, 2022
@maflcko
Copy link
Member Author

maflcko commented Nov 29, 2022

Removed it from the list in #20078 (comment)

@Sjors
Copy link
Member

Sjors commented Nov 29, 2022 •
edited
Loading

It also checks that older nodes still sync with new ones, though not in very interesting ways. But splitting it might be overkill.

Since the bulk of the code checks wallet compatibility, I don't mind renaming it. If we ever want to test more interesting (soft-fork / p2p) backwards compatibility we could also make a new test (which would ideally use MiniWallet).

@maflcko maflcko changed the title test: Move feature_backwards_compatibility.py to wallet_backwards_compatibility.py test: Move wallet tests to wallet_*.py Nov 29, 2022
@maflcko
Copy link
Member Author

maflcko commented Nov 29, 2022 •
edited
Loading

Pushed to move two wallet tests to wallet_*.py, let me know if I forgot any

@maflcko maflcko force-pushed the 2211-test-move-wallet- branch from fab57fa to faf00ed Compare November 29, 2022 17:56
@DrahtBot DrahtBot mentioned this pull request Nov 30, 2022
Closed
hernanmarino
hernanmarino reviewed Dec 5, 2022
View reviewed changes
Copy link
Contributor

@hernanmarino hernanmarino left a comment

Choose a reason for hiding this comment

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

ACK 👍

pablomartin4btc
pablomartin4btc reviewed Dec 5, 2022
View reviewed changes
Copy link
Member

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

cr ACK.
I see @Sjors's point, I still agree with this change as I'm also following the upgrading of tests to MiniWallet usage and this change conveys that intention.

@pablomartin4btc
Copy link
Member

Since we are here, perhaps you could move rpc_psbt.py to wallet_psbt.py as you mentioned here.

@maflcko
Copy link
Member Author

maflcko commented Dec 6, 2022

Not sure, about half of it is testing the wallet, the other half not. Ideally the parts were split or the non-wallet parts wouldn't depend on a wallet, so I'll leave it for a follow-up for now.

@maflcko maflcko force-pushed the 2211-test-move-wallet- branch from faf00ed to fa078f1 Compare December 6, 2022 12:20
@maflcko maflcko force-pushed the 2211-test-move-wallet- branch from fa078f1 to fa1800b Compare December 6, 2022 18:08
@DrahtBot DrahtBot mentioned this pull request Dec 8, 2022
Merged
@maflcko maflcko force-pushed the 2211-test-move-wallet- branch from fa1800b to fa7d71a Compare December 9, 2022 10:55
@maflcko
Copy link
Member Author

maflcko commented Dec 9, 2022

rebased

fanquake
fanquake approved these changes Dec 9, 2022
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK fa7d71a

pablomartin4btc
pablomartin4btc approved these changes Dec 9, 2022
View reviewed changes
Copy link
Member

@pablomartin4btc pablomartin4btc 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 fa7d71a

@maflcko maflcko merged commit 9e229a5 into bitcoin:master Dec 9, 2022
@maflcko maflcko deleted the 2211-test-move-wallet-🍗 branch December 9, 2022 15:38
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 9, 2022
Merge bitcoin#26601: test: Move wallet tests to wallet_*.py
7a3ae9e
@ryanofsky ryanofsky mentioned this pull request Dec 9, 2022
Merged
@bitcoin bitcoin locked and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fanquake fanquake fanquake approved these changes

+2 more reviewers

@hernanmarino hernanmarino hernanmarino approved these changes

@pablomartin4btc pablomartin4btc pablomartin4btc approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@maflcko @DrahtBot @Sjors @pablomartin4btc @fanquake @hernanmarino