test: use context managers and add file existence checks in feature_fee_estimation.py #31030

1. 
   AgusR7 commented at 2:24 am on October 4, 2024: none

   update feature_fee_estimation.py test script to improve resource management and exception safety:

   Use context managers for file operations: Replaced all instances of file handling using “open()” with “with open()” statements to ensure files are properly closed after their operations, even if exceptions occur.

   Add file existence checks: Before performing file operations like os.utime() and os.remove(), added checks using os.path.exists() to prevent errors when files are missing.

2. test: use context managers and add file existence checks in estimatefee.py 547663b4c1
3. test: use context managers and add file existence checks in estimatefee.py aa65f59523
4. Update feature_fee_estimation.py 41563578eb
5. test: use context managers and add file existence checks in feature_fee_estimation.py b2a9859b1d
6. 
   DrahtBot commented at 2:24 am on October 4, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept NACK	ismaelsadeeq
   Concept ACK	laanwj

   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:

   • #29500 (test: create assert_not_equal util by kevkevinpal)

   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.

7. DrahtBot added the label Tests on Oct 4, 2024
8. 
   ismaelsadeeq commented at 4:23 pm on October 4, 2024: member

   Thanks for your interest in contributing to this project @AgusR7

   I am unsure whether this PR meets the refactoring standards outlined in the contributing guidelines https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring, and I’m also not convinced that the advertised advantages justify the review effort. Therefore, I am tending towards NACK.

   If you are still willing to contribute check out the Good first issues in this project.

9. in test/functional/feature_fee_estimation.py:311 in b2a9859b1d

   307@@ -310,21 +308,24 @@ def test_old_fee_estimate_file(self):
   308         self.restart_node(0)
   309         assert_equal(self.nodes[0].estimatesmartfee(1)["feerate"], fee_rate)
   310 
   311-        fee_dat = self.nodes[0].chain_path / "fee_estimates.dat"
   312+        fee_dat = os.path.join(self.nodes[0].chain_path, "fee_estimates.dat")
   

   ---

   ---

   

   maflcko commented at 11:22 am on October 7, 2024:

   This change is in the wrong direction and seems unrelated? pathlib should be used going forward
10. 
    maflcko commented at 11:24 am on October 7, 2024: member

    You’ll have to rebase, if you are still working on this.
11. 
    laanwj commented at 11:28 am on October 21, 2024: member

    Concept ACK on using the context managers
12. 
    maflcko commented at 11:45 am on October 24, 2024: member

    Are you still working on this?
13. 
    maflcko commented at 9:04 am on November 4, 2024: member

    Closing for now, due to lack of progress. If you still want to work on this, please leave a comment to have it re-opened. Alternatively, you can open a new pull request with the outstanding feedback addressed.
14. maflcko closed this on Nov 4, 2024

    ---

Contributors
AgusR7 DrahtBot ismaelsadeeq maflcko laanwj

Labels
Tests