[CLI] Make per-run log files unique by appending timestamp + open with mode="w" (fixes #984)#83
Conversation
|
Abhishek Singh seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Code Review
This pull request updates the logging configuration to include a timestamp in log filenames and sets the file mode to write. A review comment identifies a potential portability issue on Windows systems where the 'title' parameter might contain invalid characters like colons, and provides a code suggestion to sanitize the string.
519ae4a to
8d92cd1
Compare
antonio-amjr
left a comment
There was a problem hiding this comment.
Seems good, but I would like to ask for a little change if I may.
Let me know what you think of the suggestion below.
| # filenames that are invalid on Windows (e.g. when logs are shared across | ||
| # systems for triage). | ||
| safe_title = title.replace(":", "-") | ||
| timestamp = datetime.datetime.now().strftime(_LOG_TIMESTAMP_FORMAT) |
There was a problem hiding this comment.
Can we use the .isoformat() for the timestamp?
It will have the appropriate format (YYYY-MM-DDTHH:MM:SS.ffffff) and we would use a standard approach.
| timestamp = datetime.datetime.now().strftime(_LOG_TIMESTAMP_FORMAT) | |
| timestamp = datetime.datetime.now().isoformat() |
Let me know what you think and if that works.
There was a problem hiding this comment.
Agreed — using .isoformat() makes sense here. It already provides the correct standardized format (YYYY-MM-DDTHH:MM:SS.ffffff) and keeps the implementation cleaner and more consistent.
Only one catch: isoformat() produces timestamps containing :, which makes the filename invalid on Windows (and some network/USB filesystems). This could create issues if the log file is copied to or opened on a Windows machine.
There was a problem hiding this comment.
Good catch about Windows systems.
Maybe that's not the best option.
Or maybe we could replace all : for - from the isoformat's output.
In that case, please feel free to continue with your solution if that would be a pain in Windows machines. Since my team is not using, sometimes those details may slip.
There was a problem hiding this comment.
Let me know which approach do you think would be better for all systems in this case
Fixes #984.
Root cause:
configure_logger_for_runincertification-tool-cli/th_cli/test_run/logging.pyderived the log file pathsolely from
--title(test_run_{title}.log) andloguru.logger.add(...)defaults to append mode. Reusing
-n NAMEacross runs caused successive runsto be concatenated into the same file.
Change:
_YYYY-MM-DD-HH-MM-SS) to the log filename insideconfigure_logger_for_run, so every run produces a unique file regardlessof the
-n NAMEvalue (matches the convention suggested by @OlivierGre).mode="w"tologger.add(...)as defense-in-depth, in case two runsever land on the exact same path (e.g. same second).
No public API change;
configure_logger_for_run(title)still returns theresolved path used by
run_testsat the end of execution.Verification:
-n my_run. Two distinct files inrun_logs/,each containing only its own run.