test: delete commented-out tests and add a test case in wallet_signer #33020

1. 
   naiyoma commented at 10:17 pm on July 19, 2025: contributor

   I noticed that some test cases had been commented out in wallet_signer.py. This PR uncomments and modifies some of those test cases so that they can pass.

   https://github.com/bitcoin/bitcoin/commit/dac74c69491c3787c63b7bf09a3818ea79f9b8a8 -> deleting this assertion since multiple signers is being tested here https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_signer.py#L273

   https://github.com/bitcoin/bitcoin/commit/1707c905661ef43935eaaeff06e8141f1be5ea07 test when an external signer returns malformed or invalid JSON, which would trigger this error: → https://github.com/bitcoin/bitcoin/blob/master/src/common/run_command.cpp#L42

   https://github.com/bitcoin/bitcoin/commit/b96156821efda510c262e80d38b9063fb9ab762f test importdescriptors with external signer

2. 
   DrahtBot commented at 10:17 pm on July 19, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33020.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	Sjors, theStack, achow101

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

3. naiyoma force-pushed on Jul 19, 2025
4. DrahtBot added the label CI failed on Jul 19, 2025
5. 
   DrahtBot commented at 10:25 pm on July 19, 2025: contributor

   🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/46322070708 LLM reason (✨ experimental): The CI failure is caused by a commit message formatting issue related to missing a blank line after the subject line.

   Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

   • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

   • An intermittent issue.

   Leave a comment here, if you need help tracking down a confusing failure.

6. DrahtBot removed the label CI failed on Jul 19, 2025
7. naiyoma renamed this:
   2025 2/test wallet signer
   test: add test cases to wallet_signer.py
   on Jul 20, 2025
8. DrahtBot renamed this:
   test: add test cases to wallet_signer.py
   test: add test cases to wallet_signer.py
   on Jul 20, 2025
9. DrahtBot added the label Tests on Jul 20, 2025
10. in test/functional/wallet_signer.py:82 in 38a6293fa8 outdated

    83-        # self.set_mock_result(self.nodes[1], "2")
    84-        # assert_raises_rpc_error(-1, 'Unable to parse JSON',
    85-        #     self.nodes[1].createwallet, wallet_name='not_hww2', disable_private_keys=True, external_signer=False
    86-        # )
    87-        # self.clear_mock_result(self.nodes[1])
    88+        self.set_mock_result(self.nodes[1], "0")
    

    ---

    ---

    

    Sjors commented at 8:47 am on July 21, 2025:

    38a6293fa89104f3d6f1907d04c2e4c488157307: this makes it more clear how set_mock_result is intended to be used:

    0self.set_mock_result(self.nodes[1], '0 {"invalid json"}')
    

    ---

    

    naiyoma commented at 1:32 pm on July 22, 2025:

    done https://github.com/bitcoin/bitcoin/commit/38a6293fa89104f3d6f1907d04c2e4c488157307
11. 
    Sjors commented at 9:01 am on July 21, 2025: member

    These tests have been disabled from the start in d4b0107d68a91ed4d1a5c78c8ca76251329d3f3c of #16546. So it’s good to either enable or delete them.

    For the third commit 74fb47f071451145b550062e382416db9388433d: I suggest deleting the commented out code. It’s not doing anything useful for the test, and I can’t remember why I wrote it.

12. test: external signer returns invalid JSON response 6d80e999a0
13. test: delete commented out tests da318fe53f
14. naiyoma force-pushed on Jul 22, 2025
15. 
    naiyoma commented at 1:34 pm on July 22, 2025: contributor

    For the third commit 74fb47f: I suggest deleting the commented out code. It’s not doing anything useful for the test, and I can’t remember why I wrote it. @Sjors, thanks for the review. I have also deleted this part https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_signer.py#L235

16. 
    Sjors commented at 1:34 pm on July 22, 2025: member

    ACK da318fe53fa954cc3eadd7c39e17eb3da7c3e09e
17. theStack approved
18. 
    theStack commented at 3:09 pm on July 22, 2025: contributor

    ACK da318fe53fa954cc3eadd7c39e17eb3da7c3e09e

    nit: could update the PR title to also indicate that (commented out) tests get removed

19. naiyoma renamed this:
    test: add test cases to wallet_signer.py
    test: delete commented-out tests and add a test case in wallet_signer.py
    on Jul 22, 2025
20. naiyoma renamed this:
    test: delete commented-out tests and add a test case in wallet_signer.py
    test: delete commented-out tests and add a test case in wallet_signer
    on Jul 22, 2025
21. 
    achow101 commented at 10:36 pm on July 22, 2025: member

    ACK da318fe53fa954cc3eadd7c39e17eb3da7c3e09e
22. achow101 merged this on Jul 22, 2025
23. achow101 closed this on Jul 22, 2025

    ---

Contributors
naiyoma DrahtBot Sjors theStack achow101

Labels
Tests