Skip to content
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

added log cleaner tests and fixed paths #732

Merged
merged 1 commit into from
Oct 29, 2021
Merged

Conversation

sergiud
Copy link
Collaborator

@sergiud sergiud commented Oct 29, 2021

As a preparation for #654.

@sergiud sergiud added this to the 0.6 milestone Oct 29, 2021
@google-cla google-cla bot added the cla: yes label Oct 29, 2021
@coveralls
Copy link
Collaborator

coveralls commented Oct 29, 2021

Coverage Status

Coverage increased (+3.5%) to 76.413% when pulling 3362cc6 on log-cleaner-tests into 9cf0eb7 on master.

@sergiud sergiud merged commit e8e40f7 into master Oct 29, 2021
@sergiud sergiud deleted the log-cleaner-tests branch October 29, 2021 21:14
dirs = GetLoggingDirectories();
} else {
size_t pos = base_filename.find_last_of(possible_dir_delim, 0,
sizeof(possible_dir_delim));
Copy link
Contributor

@aesophor aesophor Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @sergiud, I think the arguments passed to find_last_of is incorrect:

  • line 1333-1334 would only search base_filename for any char in possible_dir_delim starting at index 0 for n characters, where n = sizeof(possible_dir_delim) (and in this case n = 1)
  • I think only the first argument is needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Do you have a specific test case in mind that triggers the problem? Or even better, could you submit directly a PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I have one!

I called SetLogDestination(INFO, “testdir/”) and came across this problem. I’ll submit a PR.

#include <glog/logging.h>

int main(int argc, char *argv[]) {
  google::InitGoogleLogging(argv[0]);
  google::SetLogDestination(google::INFO, "testdir/");
  google::EnableLogCleaner(1);
  LOG(INFO) << "msg";
}

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

Successfully merging this pull request may close these issues.

3 participants