Fixed IsLogFromCurrentProject to include all severities#1062
Fixed IsLogFromCurrentProject to include all severities#1062Mivekk wants to merge 1 commit intogoogle:masterfrom
Conversation
sergiud
left a comment
There was a problem hiding this comment.
Thanks for the PR. I have a couple of remarks.
There seem to be build failures caused by the changes.
Please also update the cleaner test cases by adding the corresponding log output.
src/logging.cc
Outdated
| cleaned_base_filename.pop_back(); | ||
| int dot_pos = cleaned_base_filename.find_last_of('.'); | ||
| cleaned_base_filename.erase(dot_pos + 1, std::string::npos); |
There was a problem hiding this comment.
You do actually need to modify the string. Something like this should be sufficient:
| cleaned_base_filename.pop_back(); | |
| int dot_pos = cleaned_base_filename.find_last_of('.'); | |
| cleaned_base_filename.erase(dot_pos + 1, std::string::npos); | |
| const string::size_type dot_pos = cleaned_base_filename.find_last_of('.', cleaned_base_filename.size() - 1); |
src/logging.cc
Outdated
| bool found_file = false; | ||
| for (const auto& severity : LogSeverityNames) { | ||
| string cleaned_base_filename_with_severity = cleaned_base_filename + severity; |
There was a problem hiding this comment.
| bool found_file = false; | |
| for (const auto& severity : LogSeverityNames) { | |
| string cleaned_base_filename_with_severity = cleaned_base_filename + severity; | |
| ```suggestion | |
| const string base_filename = cleaned_base_filename.substring(0, dot_pos); | |
| bool found_file = false; | |
| for (const auto& severity : LogSeverityNames) { | |
| string cleaned_base_filename_with_severity = base_filename + severity; |
692680d to
4e016e5
Compare
|
I'm not sure how I should modify cleaner test cases unfortunately |
|
You can extend any of the glog/src/cleanup_immediately_unittest.cc Lines 62 to 64 in ac12a9e by additionally logging at the missing severities. Please note that the unit tests need to be updated either way because these are failing now with your changes. |
4e016e5 to
6ea4aea
Compare
|
Updated unit test and all checks should pass now hopefully |
|
Unfortunately, the cleanup tests are still failing. |
6ea4aea to
71a04a1
Compare
|
Fixed an issue different way |
|
The unit tests are still failing though. |
|
Closing as abandoned. Please let me know if you want to continue working on this PR. |
Previously when executing following code LogCleaner would only delete overdue Error log files:
And if we changed the order to for example:
LogCleaner would only delete overdue Info log files and overdue Error and Warning files were ignored.
To include all severities during LogCleaner's sweep I tested filepath for all severities in a IsLogFromCurrentProject function.