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

Fix flakiness in log rotation test #2213

Merged

Conversation

m3co-code
Copy link
Contributor

This PR fixes #2203 by waiting up to 3 seconds until the new log file is created after the signal was sent to Traefik to rotate the log files. Also, it improves logging of the log file output in case something is not correct to ease the detection of potential other problems that were hidden before.

@traefiker traefiker added this to the 1.4 milestone Oct 4, 2017
@m3co-code m3co-code force-pushed the fix-flakiness-in-log-rotation-test branch from 4f5f0ab to 0b3ba87 Compare October 4, 2017 15:19
Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Some suggestions, nothing biggie.

@@ -111,6 +120,13 @@ func (s *LogRotationSuite) TestTraefikLogRotation(c *check.C) {
c.Assert(lineCount, checker.GreaterOrEqualThan, 7)
}

func logAccessLogFile(c *check.C, fileName string) {
output, err := ioutil.ReadFile(fileName)
c.Assert(err, checker.IsNil, check.Commentf("error opening access log file %s: %s", fileName, err))
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR the error from ioutil.ReadFile will already contain the file name and detailed error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I removed the Commentf.

output, err := ioutil.ReadFile(fileName)
c.Assert(err, checker.IsNil, check.Commentf("error opening access log file %s: %s", fileName, err))
c.Logf("Contents of file %s", fileName)
c.Log(string(output))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also be part of the previous statement, separating the message "header" and the content by a newline (\n).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified it into one line 👍

@@ -111,6 +120,13 @@ func (s *LogRotationSuite) TestTraefikLogRotation(c *check.C) {
c.Assert(lineCount, checker.GreaterOrEqualThan, 7)
}

func logAccessLogFile(c *check.C, fileName string) {
output, err := ioutil.ReadFile(fileName)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a case of t.Helper()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case of the integration tests we can not use it, as we don't have access to testing.T.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah -- sad and true.

@m3co-code
Copy link
Contributor Author

@timoreimann thanks for the review, I incorporated the feedback. I am not sure though why Semaphore won't build this branch..

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

LGTM. 👏

@ldez
Copy link
Contributor

ldez commented Oct 4, 2017

@ldez
Copy link
Contributor

ldez commented Oct 4, 2017

I close and reopen due to Semaphore

@ldez ldez closed this Oct 4, 2017
@ldez ldez reopened this Oct 4, 2017
Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 👏

m3co-code and others added 3 commits October 6, 2017 07:08
- wait up to 3 seconds for the creation of the new log file after
  sending the signal to rotate the log files
- improve logging output in case of failure
@traefiker traefiker force-pushed the fix-flakiness-in-log-rotation-test branch from 5e616b8 to 1f36ff0 Compare October 6, 2017 07:08
@traefiker traefiker merged commit 9db8773 into traefik:v1.4 Oct 6, 2017
@ldez ldez changed the title fix flakiness in log rotation test Fix flakiness in log rotation test Oct 10, 2017
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.

5 participants