Fix issues with the Bump Allocator & Logging tests for Windows CI#59
Conversation
* Fixes issues with the Bump Allocator & Logging tests for Windows CI. * For the Bump Allocator test, we needed to update the test to check if a second page was allocated or if the first page simply had enough capacity to satisfy both requirements due to OS rounding. * For the Logging test, we needed to ensure that `shutdown()` is called inside a cleanup block so that it runs even if an exception is thrown. Furthermore, we need to call `shutdown()` before attempting to delete the log file to guarantee that the background thread has completely dropped its file handles.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes update two tests: logging now shuts down before file validation, and the bump allocator test accepts either a new page or sufficient first-page growth for the second allocation. ChangesLogging test lifecycle
Allocator test portability
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
OldDev78
left a comment
There was a problem hiding this comment.
Good. I would even consider adding actual destructors to prevent this kind of mistakes in the future.
Enough of this irrational prejudice for "implicit behavior".
Having control is not the same as requiring boilerplate everywhere.
shutdown()is called inside a cleanup block so that it runs even if an exception is thrown. Furthermore, we need to callshutdown()before attempting to delete the log file to guarantee that the background thread has completely dropped its file handles.Summary by CodeRabbit