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

net/http: test spams stderr #24831

Closed
bradfitz opened this issue Apr 12, 2018 · 10 comments

Comments

Projects
None yet
5 participants
@bradfitz
Copy link
Member

commented Apr 12, 2018

Running go test on a package should be quiet and not spam to stderr.

But:

$ go test -short
2018/04/12 17:09:17 http: WriteHeader called with X-Content-Type-Options:nosniff but no Content-Type
2018/04/12 17:09:17 http: TLS handshake error from 127.0.0.1:55706: remote error: tls: bad certificate
PASS
ok      net/http        4.034s

The first log line was added in 1a677e0 (/cc mikesamuel).

I haven't looked into the second.

Help fixing these would be great. Thanks!

@bradfitz bradfitz added this to the Go1.11 milestone Apr 12, 2018

@odeke-em

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

Thank you for this report @bradfitz

For further investigation, I'll kindly page:

  1. /cc @mikesamuel -- @bradfitz you forgot the @ :)
  2. Looks like a regression from c529141 /cc @mikedanese @FiloSottile @agl
@mikesamuel

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2018

Sorry. The first one's my fault. I'll look into it. The second looks unlikely to be caused by what I did to cause the first.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

@mikesamuel no worries :) In deed, the second one was caused by a different commit

Looks like a regression from c529141 /cc @mikedanese @FiloSottile @agl

@gopherbot

This comment has been minimized.

Copy link

commented Apr 13, 2018

Change https://golang.org/cl/106756 mentions this issue: net/http: log output from serve_test reached stderr

@mikedanese

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2018

Haven't looked deeply into this yet. @odeke-em can you give me a hint on what lead you to c529141?

@mikedanese

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2018

I don't think c529141 is related. More likely 2638001.

gopherbot pushed a commit that referenced this issue Apr 16, 2018

net/http: remove some stderr log spam
This changes rawResponse to install a logger before
Serve()ing and makes the log output available to
tests.

Updates #24831
Updates CL 89275

Change-Id: I0fb636a35b05959ca9978d5d8552f38b7cf8e8b5
Reviewed-on: https://go-review.googlesource.com/106756
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@odeke-em

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

Sorry for the late reply @mikedanese, I've been travelling and was away from my computer for a bit. I went through the history https://github.com/golang/go/commits/master/src/crypto and commits before and after yours after some bisection using some hacked up bash script to do binary searches, recompile the Go tree and then find regressions(but it isn't yet complete so not yet releasing it ;))

@mikedanese you can try to confirm it yourself, the commit before yours 8a15192 didn't produce that result but yours c529141 did and so did the commit after yours 0b37f05
screen shot 2018-04-16 at 7 45 57 pm

@mikedanese

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2018

From my previous comment, I've ruled out c529141. You can confirm this by reverting that change, running tests, and still seeing the error output. The test added in 2638001 is creating the output, and has been creating the output since it was merged.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Apr 17, 2018

@mikedanese thank you for the thorough investigation, I was just following changes in crypto, my apologies for the spurious git blame.

In that case /cc @pquerna and @FiloSottile.

@gopherbot

This comment has been minimized.

Copy link

commented Apr 18, 2018

Change https://golang.org/cl/107817 mentions this issue: net/http: remove stderr log spam in test

@gopherbot gopherbot closed this in 1473789 Apr 18, 2018

@golang golang locked and limited conversation to collaborators Apr 18, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.