-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
logging: update header timestamp on rollover #6546
Conversation
@@ -238,15 +239,22 @@ def do_rollover(self) -> None: | |||
os.dup2(self.stream.fileno(), sys.stderr.fileno()) | |||
# Emit header records (should only do this for subsequent log files) | |||
for header_record in self.header_records: | |||
now = time() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's entirely reasonable not to test assigning the output of a standard library function, and marking this as not needing testing gives us a better idea of what is and isn't tested.
now = time() | |
now = time() # pragma: no cover |
I'm now questioning whether Cov should even pick this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, codecode says this is covered:
codecov/patch — 100.00% of diff hit (target 97.00%)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatev. Not super important. Wierd though. I think that you can merge this on one review if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Read code.
- Manually tested.
Doesn't have a unit test, but perhaps the game isn't worth the candle for this change.
When the scheduler log rolls over, the header lines get logged with the original timestamp rather than the rollover time which is a tad confusing.
Test using:
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).?.?.x
branch.