test: Remove unused try-block in assert_debug_log #16804

1. 
   MarcoFalke commented at 5:15 PM on September 4, 2019: member

   This try block has accidentally been added by me in fa3e9f7627784ee00980590e5bf044a0e1249999. It was unused all the time, but commit 6011c9d72d1df5c2cd09de6f85c21eb4f7eb1ba8 added a return in the finally block, muting all exceptions.

   This can be tested by adding an assert False after any with ...assert_debug_log...: line.

2. test: Remove incorrect and unused try-block in assert_debug_log fae91a09c4
3. MarcoFalke added this to the milestone 0.19.0 on Sep 4, 2019
4. DrahtBot added the label Tests on Sep 4, 2019
5. ryanofsky approved
6. 
   ryanofsky commented at 5:59 PM on September 4, 2019: member

   utACK fae91a09c453a9a95c382df765bd71e54698d5b2. I didn't know returning inside a finally block would cancel pending exceptions or return values, but I guess this makes sense and is a good thing to be aware of.

   I'd maybe rephrase "Remove incorrect and unused try-block". I think the try block was fine and even potentially useful, but the return statement was wrong. It should be fine to remove the try block, though. Only difference should be that if a test is expecting an exception and a log message it will now need to catch the exception inside the assert_debug_log block instead of being able to catch it outside.

7. 
   DrahtBot commented at 6:32 PM on September 4, 2019: member

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #16673 (Relog configuration args on debug.log rotation by LarryRuane)
   • #16115 (On bitcoind startup, write config args to debug.log by LarryRuane)

   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.

8. 
   MarcoFalke commented at 11:48 PM on September 4, 2019: member

   I think the try block was fine and even potentially useful, but the return statement was wrong

   I think either of them is fine on their own, but in combination, at least one of them is incorrect. Though, I am happy to change to something else, if you have any suggestions.

9. 
   ryanofsky commented at 12:24 AM on September 5, 2019: member

   It's fine, I only meant to suggest changing "Remove incorrect and unused try-block" to "Remove unused try-block." AFAICT using a try block is correct and appropriate here, but nothing requires a try block right now, so it's ok to simplify and remove. But returning in a final block ignores exceptions, so it's definitely a bug to use a return in a final block if you don't want to ignore exceptions.

10. MarcoFalke renamed this:
    test: Remove incorrect and unused try-block in assert_debug_log
    test: Remove unused try-block in assert_debug_log
    on Sep 5, 2019
11. 
    MarcoFalke commented at 12:27 AM on September 5, 2019: member

    Renamed GitHub title

12. 
    laanwj commented at 11:27 AM on September 5, 2019: member

    ACK fae91a09c453a9a95c382df765bd71e54698d5b2

13. laanwj referenced this in commit cbde2bc806 on Sep 5, 2019
14. laanwj merged this on Sep 5, 2019
15. laanwj closed this on Sep 5, 2019

    ---

16. MarcoFalke deleted the branch on Sep 5, 2019
17. sidhujag referenced this in commit 645d8a7a43 on Sep 10, 2019
18. deadalnix referenced this in commit 8c5a3b06e7 on May 2, 2020
19. vijaydasmp referenced this in commit 25d755477a on Nov 17, 2021
20. vijaydasmp referenced this in commit e647dc9207 on Nov 21, 2021
21. vijaydasmp referenced this in commit 6f07a4c4ce on Nov 21, 2021
22. vijaydasmp referenced this in commit 4a656b4e52 on Nov 22, 2021
23. vijaydasmp referenced this in commit 7874e7852e on Nov 25, 2021
24. DrahtBot locked this on Dec 16, 2021

Contributors

MarcoFalkeryanofskyDrahtBotlaanwj

Milestone
0.19.0