test: add unit tests for SigningResultString function #31567

1. 
   lucasbalieiro commented at 9:19 pm on December 24, 2024: none

   This PR introduces new unit tests for the SigningResultString function located in the file src/common/signmessage.cpp.

   Motivation:

   Prior to this PR, the SigningResultString function had no test coverage. This update adds the necessary test cases to ensure that the function behaves as expected.

   Changes:

   • Added unit tests to cover all the cases in the switch statement within the SigningResultString function.
   • Ensured that each valid SigningResult enum value is tested for the expected output string:
     • SigningResult::OK
     • SigningResult::PRIVATE_KEY_NOT_AVAILABLE
     • SigningResult::SIGNING_FAILED
   • Placed the new tests in the file location that matches the context of other related tests, ensuring consistency in test organization.

   Suggested Follow-Up:

   The current implementation of the function includes an assert(false) for invalid enum values, as there is no default case in the switch statement. Since modifying the original function is not within the scope of this PR, perhaps someone with more C++ experience could explore writing a test to verify the behavior of the assert and the compiler warnings referenced in the comment:

   // no default case, so the compiler can warn about missing cases

2. test: add unit tests for SigningResultString function 6d4536afe8
3. 
   DrahtBot commented at 9:19 pm on December 24, 2024: 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/31567.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

4. DrahtBot added the label Tests on Dec 24, 2024
5. 
   maflcko commented at 8:09 am on December 25, 2024: member

   No objection, but I also don’t see the point of adding a unit test for a trivial function that stringifies an enum class. Also, I am not sure about adding 3 lines of test code with a pull request description that is AI generated and mostly just repeats the changes in words.

   Adding test coverage is great, but I think here it would be better to add it as an RPC test, so that it is clear how to reach each code path from real code, by a real user. See the RPC coverage: https://maflcko.github.io/b-c-cov/total.coverage/src/wallet/rpc/signmessage.cpp.gcov.html . Otherwise, adding a test for this may be problematic for various reasons:

   • If the code is changed, the exact change needs to be mirrored in the unit test, for an unclear benefit.
   • If the code is (or becomes) dead code, the unit test will hide this fact and make it harder to detect dead code.
6. 
   brunoerg commented at 10:03 am on December 25, 2024: contributor

   Concept ~0. I agree with @maflcko, I prefer to have it as an RPC test, I don’t see a strong value on having a unit test for this trivial thing.
7. 
   lucasbalieiro commented at 11:20 am on December 25, 2024: none

   @maflcko and @brunoerg , agreed. Thanks for the time. Closing the PR.
8. lucasbalieiro closed this on Dec 25, 2024

   ---

9. lucasbalieiro deleted the branch on Dec 25, 2024

Contributors
lucasbalieiro DrahtBot maflcko brunoerg

Labels
Tests