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.

Sign up for GitHub

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

Jump to bottom

Fix integration tests in tracing #2759

Merged
merged 1 commit into from Jan 25, 2018
Merged

Fix integration tests in tracing #2759

merged 1 commit into from Jan 25, 2018

Conversation

mmatur
Copy link
Member

@mmatur mmatur commented Jan 24, 2018

What does this PR do?

This PR fix flaky integration tests in tracing.

Flaky was due to bug in the latest Zipkin version 2.4.4

More

  • Added/updated tests

Additional Notes

screenshot from 2018-01-24 17-03-31

@mmatur mmatur added this to the 1.6 milestone Jan 24, 2018
ldez
ldez approved these changes Jan 24, 2018
View reviewed changes
Copy link
Member

@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

nmengin
nmengin approved these changes Jan 24, 2018
View reviewed changes
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

@mjeri
Copy link
Contributor

mjeri commented Jan 25, 2018

I don't understand the PR. From the description I read it as this should fix a flaky integration test but it actually adds a new one. What am I missing?

@mmatur
Copy link
Member Author

mmatur commented Jan 25, 2018

@marco-jantke In the merge back fron 1.5 into master #2754. We have decided to remove tracing_test.go because integration tests was failing every time due to an issue in the Zipkin docker image. And we wanted to have times to understand and fix properly these tests. This this why we have re add tracing_test.go

mjeri
mjeri approved these changes Jan 25, 2018
View reviewed changes
Copy link
Contributor

@mjeri mjeri left a comment

Choose a reason for hiding this comment

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

Ok, I see. Thanks for explaining. So I assume that the previous version of the integration test already went through a normal code-review procedure and that only the version number was updated. So I give my LGTM without looking detailed into the code.

Please let me know in case a more thorough review is needed still :)

@traefiker traefiker merged commit 9f741ab into traefik:master Jan 25, 2018
@mmatur mmatur deleted the fix/tracing-it-tests branch March 21, 2018 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjeri mjeri mjeri approved these changes

@ldez ldez ldez approved these changes

@nmengin nmengin nmengin approved these changes

Assignees
No one assigned
Labels
area/middleware/tracing kind/bug/fix a bug fix size/M
Projects
None yet
Milestone
1.6
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mmatur @mjeri @ldez @nmengin @traefiker