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

Display file log when test fails. #2801

Merged
merged 2 commits into from Feb 5, 2018
Merged

Conversation

ldez
Copy link
Member

@ldez ldez commented Feb 3, 2018

What does this PR do?

Display file log when test fails.

Clean some test methods.

Motivation

Have more information when tests fails.

More

  • Added/updated tests

@ldez ldez added this to the 1.6 milestone Feb 3, 2018
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 🗞️

Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Given the fact that we use go-check, wouldn't it make more sense to do that in a TearDown or TearDownSuite method for the suite(s) ? It would remove some duplication and would help not forget to add it in new tests.

(if using testing instead of go-check, I think it would be best in TestMain 😛)

@ldez
Copy link
Member Author

ldez commented Feb 5, 2018

@vdemeester I already tested that:

  • we already use TearDown or TearDownSuite for some tests: it's not possible to have 2 methods
  • this need to create a state, and that create some other problems.
  • In some tests we need to read the buffer in the test.
  • we need the test state

Edit: this concern only the console output but it's not a problem for the log file

@@ -19,8 +19,8 @@ import (
checker "github.com/vdemeester/shakers"
)

var integration = flag.Bool("integration", false, "run integration tests")
var container = flag.Bool("container", false, "run container integration tests")
var integration = flag.Bool("integration", true, "run integration tests")
Copy link
Member

Choose a reason for hiding this comment

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

Could you please change to false

var integration = flag.Bool("integration", false, "run integration tests")
var container = flag.Bool("container", false, "run container integration tests")
var integration = flag.Bool("integration", true, "run integration tests")
var container = flag.Bool("container", true, "run container integration tests")
Copy link
Member

Choose a reason for hiding this comment

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

Could you please change to false

Copy link
Member Author

Choose a reason for hiding this comment

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

oops

Copy link
Member

@mmatur mmatur 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

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@traefiker traefiker merged commit dcba74d into traefik:master Feb 5, 2018
@ldez ldez deleted the refactor/access-log branch February 5, 2018 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/logs kind/enhancement a new or improved feature. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants