Skip to content

test: create default wallet in extended tests#19971

Merged
fanquake merged 1 commit intobitcoin:masterfrom
Sjors:2020/09/fix_no_default_wallet_extended_tests
Sep 19, 2020
Merged

test: create default wallet in extended tests#19971
fanquake merged 1 commit intobitcoin:masterfrom
Sjors:2020/09/fix_no_default_wallet_extended_tests

Conversation

@Sjors
Copy link
Member

@Sjors Sjors commented Sep 18, 2020

This was omitted from #15454

@Sjors Sjors mentioned this pull request Sep 18, 2020
Merged
@fanquake fanquake added the Tests label Sep 18, 2020
@Sjors Sjors mentioned this pull request Sep 18, 2020
Closed
@jnewbery
Copy link
Contributor

jnewbery commented Sep 18, 2020

Can these be fixed by adding the -wallet= command line argument, like the other tests in #15454?

@promag
Copy link
Contributor

promag commented Sep 18, 2020

@jnewbery if we ditch automatic wallet creation then we could already avoid that approach.

@Sjors Sjors force-pushed the 2020/09/fix_no_default_wallet_extended_tests branch from 9df44e0 to 165b8fd Compare September 18, 2020 12:12
@Sjors
Copy link
Member Author

Sjors commented Sep 18, 2020

@jnewbery done

@jnewbery
Copy link
Contributor

jnewbery commented Sep 18, 2020

utACK 165b8fd561

Thanks for the quick fix!

ryanofsky
ryanofsky approved these changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK 165b8fd5617e4bd4d5f99586306a66f084d95d13

test/functional/feature_dbcrash.py Outdated
Copy link
Contributor

@ryanofsky ryanofsky Sep 18, 2020

Choose a reason for hiding this comment

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

In commit "test: create default wallet in extended tests" (165b8fd5617e4bd4d5f99586306a66f084d95d13)

Could you add a comment here? It's unclear why you'd want to call the same function n times.

Copy link
Member Author

@Sjors Sjors Sep 18, 2020

Choose a reason for hiding this comment

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

That's a leftover bug from my refactor.

@ryanofsky
Copy link
Contributor

ryanofsky commented Sep 18, 2020

Previous version 9df44e0b97ba74b4df01e600b4c19e9d75310498 also looked good, but John's suggestion to use -wallet="" is good because it provides more consistency with other tests so they be cleaned up easier later. There are a few ways tests could conceivably create default wallets:

  1. Not pass any -wallet arguments or call createwallet RPC, just rely on bitcoind default wallet creation (only possible before Remove the automatic creation and loading of the default wallet #15454)
  2. Pass ["-wallet="] argument and rely on test framework to strip out the argument and turn it into a createwallet RPC call behind the scenes
  3. Just call createwallet RPC directly from the test
  4. Give test framework a wants_default_wallet or similar test setup option, so tests have a more direct way of saying whether they want a default wallet or not and we can get rid of the "-wallet=" interception stuff.

#15454 moved us from (1) to (2). Originally this PR used approach (3) for extended tests, now it uses (2). Probably we want to end up doing (4)

@Sjors Sjors force-pushed the 2020/09/fix_no_default_wallet_extended_tests branch from 165b8fd to a5f5374 Compare September 18, 2020 15:55
@Sjors
Copy link
Member Author

Sjors commented Sep 18, 2020

Third time's a charm.

ryanofsky
ryanofsky approved these changes Sep 18, 2020
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK a5f5374. Just reverted a leftover diff since last review

glozow
glozow reviewed Sep 18, 2020
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

utACK a5f5374

@fanquake fanquake merged commit 967be53 into bitcoin:master Sep 19, 2020
@Sjors Sjors deleted the 2020/09/fix_no_default_wallet_extended_tests branch September 19, 2020 15:21
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 7, 2021
@achow101 @PiRK
Remove the automatic creation and loading of the default wallet
4078287 
Summary:
No longer create a default wallet. The default wallet will still be
loaded if it exists and not other wallets were specified (anywhere,
including settings.json, bitcoin.conf, and command line).

Tests are updated to be started with `-wallet=` if they need the default
wallet.

Added test to wallet_startup.py testing that no default wallet is
created and that it is loaded if it exists and no other wallets were
specified.

Tell users how to load or create a wallet when no wallet is loaded

This is a backport of [[bitcoin/bitcoin#15454 | core#15454]] and  [[bitcoin/bitcoin#19971 | core#19971]] (bug fix)

Depends on D10240

Test Plan: `ninja all check check-functional-extended`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10243
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+1 more reviewer

@glozow glozow glozow left review comments

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

@Sjors @jnewbery @promag @ryanofsky @glozow @fanquake