Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

alwaysIncludePattern can cause an uncaught exception #134

Closed
issacg opened this Issue May 9, 2013 · 2 comments

Comments

Projects
None yet
2 participants
Contributor

issacg commented May 9, 2013

I caught another bug in #132 (more mine, this time ;)) which can happen with alwaysIncludePattern.

The existing roll code didn't handle errors in renameTheFile. If the oldfile == newfile (which is exactly what happens if we renamed the file when we opened it, rather than when we rolled it) we'd pass the error on and cause openTheStream to not open, and write to a closed stream. If oldfile != newfile, we'd roll and not write to a closed stream, but we'd still change the filename again, which is also not correct.

I patched this in issacg:dontalwaysrename-bug but didn't write a test (yet).

To do so, I'd need to tinker with the time with sinon, timekeeper or the like (I'm familiar with those 2), to keep track of what gets rolled, when, and what files should exist during the test. So before committing a pull request, I'd like to hear how you'd want it dealt with.

My thought was to do a seconds-based rollover pattern in a controlled way to test both of the above scenarios. Thoguhts?

Owner

nomiddlename commented May 9, 2013

The existing tests for streams/DateRollingFileStream pass in a function that can be used as a replacement for "Date.now()", that should allow you to control the rolling of the file.

Owner

nomiddlename commented May 16, 2013

I merged in your fix and published v0.6.5 to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment