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

logcollector #361

Merged
merged 7 commits into from
Oct 10, 2014
Merged

logcollector #361

merged 7 commits into from
Oct 10, 2014

Conversation

cgzones
Copy link
Contributor

@cgzones cgzones commented Oct 9, 2014

  • fix some compiler warnings
  • fix some static analyzer warnings

@awiddersheim
Copy link
Member

Hrm, looks like the tests failed here:

https://travis-ci.org/ossec/ossec-hids/jobs/37510615#L2778

Looks like this test is supposed to fail:

https://github.com/ossec/ossec-hids/blob/master/src/tests/test_os_net.c#L288

I'm not really familiar with travis-ci or the tests. This seems like a one off failure? Did just this test timeout or all the tests as a whole hit some type of timeout?

I'd merge but since the tests hit a failure I don't think all the tests ran after this one. Is there a way to kick off another build without committing?

@awiddersheim
Copy link
Member

Researched a bit more. Here is my understanding. I'm not seeing a timeout being set in our set of tests (could have just missed it) so the default is being set which according to this documentation is four seconds:

http://check.sourceforge.net/doc/check_html/check_4.html#Test-Timeouts

The test against OS_GetHost() specifies a retry attempt of two:

https://github.com/ossec/ossec-hids/blob/master/src/tests/test_os_net.c#L288

Looking at the OS_GetHost() function this actually ends up being three attempts sleeping a second between each attempt so one can expect this test to take at least three seconds. I'm not sure how long a call to gethostbyname() might take but this cuts kind of close to that default timeout and lowering the attempts in the test might be in order.

Hopefully, my interpretation of all of this is makes sense but I'd love to learn more so please let me know.

@cgzones
Copy link
Contributor Author

cgzones commented Oct 10, 2014

Thanks for your investigation @awiddersheim.
I rebased this branch, so let us see if it fails again.
About the unit test: i will think over it

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