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

test: use CheckRespStatus(c, resp, code) to add comment when failure #684

Merged

Conversation

allencloud
Copy link
Collaborator

Signed-off-by: Allen Sun [email protected]

1.Describe what this PR did

In issue #673, we found that some integration test fails without showing any error message, I think it is very unreasonable for us to debug and bugfix.

So, I think we need to use CheckRespStatus(c, resp, code ) to check status code and should error if this checking failed.

However, we still have one potential issue:

// CheckRespStatus checks the http.Response.Status is equal to status.
func CheckRespStatus(c *check.C, resp *http.Response, status int) {
	if resp.StatusCode != status {
		got := types.Error{}
		_ = request.DecodeBody(&got, resp.Body)
		c.Assert(resp.StatusCode, check.Equals, status, check.Commentf("Error:%s", got.Message))
	}
}

In the code above if we assert CheckRespStatus(c, resp, 500), we mean error is supposed. But if the actual status code is 200, then the message is not so readable. But it is OK, I think.

2.Does this pull request fix one issue?
related to #673 to find more test failure cases.

3.Describe how you did it
None

4.Describe how to verify it
None

5.Special notes for reviews
/cc @Letty5411

@pouchrobot pouchrobot added areas/test kind/bug This is bug report for project size/M labels Feb 2, 2018
@Letty5411
Copy link
Contributor

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Feb 2, 2018
@codecov-io
Copy link

Codecov Report

Merging #684 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #684   +/-   ##
=======================================
  Coverage   10.01%   10.01%           
=======================================
  Files          92       92           
  Lines        5353     5353           
=======================================
  Hits          536      536           
  Misses       4767     4767           
  Partials       50       50

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0258015...7bfacd1. Read the comment docs.

@allencloud allencloud merged commit 4b6bb67 into AliyunContainerService:master Feb 2, 2018
@allencloud allencloud deleted the user-CheckRespStatus branch February 2, 2018 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/test kind/bug This is bug report for project LGTM one maintainer or community participant agrees to merge the pull reuqest. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants