Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(456)

Issue 108170043: code review 108170043: "Testing Techniques" talk (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 6 months ago by adg
Modified:
6 years, 5 months ago
Reviewers:
r, campoy, gmlewis1
CC:
dsymonds, crawshaw1, gmlewis1, r, Sameer at Google, bcmills, campoy, golang-codereviews
Visibility:
Public.

Description

"Testing Techniques" talk

Patch Set 1 #

Patch Set 2 : diff -r c636a8a8316f https://code.google.com/p/go.talks #

Total comments: 32

Patch Set 3 : diff -r c636a8a8316f https://code.google.com/p/go.talks #

Patch Set 4 : diff -r c636a8a8316f https://code.google.com/p/go.talks #

Total comments: 7

Patch Set 5 : diff -r 3bc49c11e4c5 https://code.google.com/p/go.talks #

Patch Set 6 : diff -r 3bc49c11e4c5 https://code.google.com/p/go.talks #

Patch Set 7 : diff -r 3bc49c11e4c5 https://code.google.com/p/go.talks #

Patch Set 8 : diff -r 3bc49c11e4c5 https://code.google.com/p/go.talks #

Total comments: 25

Patch Set 9 : diff -r 3bc49c11e4c5 https://code.google.com/p/go.talks #

Patch Set 10 : diff -r 7094cf573469 https://code.google.com/p/go.talks #

Unified diffs Side-by-side diffs Delta from patch set Stats (+405 lines, -0 lines) Patch
A 2014/testing.slide View 1 chunk +259 lines, -0 lines 0 comments Download
A 2014/testing/cover.png View Binary file 0 comments Download
A 2014/testing/go1.1.png View Binary file 0 comments Download
A 2014/testing/httprecorder.go View 1 chunk +26 lines, -0 lines 0 comments Download
A 2014/testing/httpserver.go View 1 chunk +31 lines, -0 lines 0 comments Download
A 2014/testing/subprocess/subprocess.go View 1 chunk +11 lines, -0 lines 0 comments Download
A 2014/testing/subprocess/subprocess_test.go View 1 chunk +21 lines, -0 lines 0 comments Download
A 2014/testing/test1/string_test.go View 1 chunk +14 lines, -0 lines 0 comments Download
A 2014/testing/test2/string_test.go View 1 chunk +43 lines, -0 lines 0 comments Download

Messages

Total messages: 22
dsymonds
https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slide File 2014/testing/testing.slide (right): https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slide#newcode43 2014/testing/testing.slide:43: t.Skipf("this doesn't work on ARM") t.Skip?
6 years, 6 months ago (2014-06-25 02:47:25 UTC) #1
crawshaw1
https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slide File 2014/testing/testing.slide (right): https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slide#newcode176 2014/testing/testing.slide:176: This means they can access unexported details, as we ...
6 years, 6 months ago (2014-06-25 04:40:11 UTC) #2
gmlewis1
LGTM! Just a few minor nits. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slide File 2014/testing/testing.slide (right): https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slide#newcode226 2014/testing/testing.slide:226: In your tests, ...
6 years, 6 months ago (2014-06-25 05:34:03 UTC) #3
r
https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slide File 2014/testing/testing.slide (right): https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slide#newcode12 2014/testing/testing.slide:12: Go has a built in testing framework. built-in https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slide#newcode16 ...
6 years, 6 months ago (2014-06-25 06:15:16 UTC) #4
Sameer at Google
https://codereview.appspot.com/108170043/diff/20001/2014/testing/subprocess/subprocess_test.go File 2014/testing/subprocess/subprocess_test.go (right): https://codereview.appspot.com/108170043/diff/20001/2014/testing/subprocess/subprocess_test.go#newcode17 2014/testing/subprocess/subprocess_test.go:17: if e, ok := err.(*exec.ExitError); ok && !e.Success() { ...
6 years, 6 months ago (2014-06-25 14:07:16 UTC) #5
adg
https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slide File 2014/testing/testing.slide (right): https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slide#newcode12 2014/testing/testing.slide:12: Go has a built in testing framework. On 2014/06/25 ...
6 years, 6 months ago (2014-06-25 16:41:52 UTC) #6
bcmills
https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slide File 2014/testing/testing.slide (right): https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slide#newcode42 2014/testing/testing.slide:42: if runtime.GOARM == "arm" { runtime.GOARCH https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slide#newcode290 2014/testing/testing.slide:290: * ...
6 years, 6 months ago (2014-06-25 17:12:04 UTC) #7
r
https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slide File 2014/testing/testing.slide (right): https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slide#newcode36 2014/testing/testing.slide:36: And enabling parallel tests: i agree with dvyukov that ...
6 years, 6 months ago (2014-06-25 17:12:12 UTC) #8
r
Thanks for serendipitously backing me on mocks, bcmills. -rob On Wed, Jun 25, 2014 at ...
6 years, 6 months ago (2014-06-25 17:13:04 UTC) #9
campoy
On 2014/06/25 17:13:04, r wrote: > Thanks for serendipitously backing me on mocks, bcmills. > ...
6 years, 6 months ago (2014-07-11 23:46:30 UTC) #10
adg
On 12 July 2014 09:46, <campoy@google.com> wrote: > was this ever submitted? No. These are ...
6 years, 6 months ago (2014-07-14 00:40:07 UTC) #11
adg
Hello dsymonds@golang.org, crawshaw@google.com, gmlewis@google.com, r@golang.org, sameer@google.com, bcmills@google.com, campoy@google.com (cc: golang-codereviews@googlegroups.com), I'd like you to review ...
6 years, 5 months ago (2014-08-04 06:43:48 UTC) #12
gmlewis1
On 2014/08/04 06:43:48, adg wrote: > Hello mailto:dsymonds@golang.org, mailto:crawshaw@google.com, mailto:gmlewis@google.com, > mailto:r@golang.org, mailto:sameer@google.com, mailto:bcmills@google.com, mailto:campoy@google.com ...
6 years, 5 months ago (2014-08-04 15:41:40 UTC) #13
r
LGTM https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide File 2014/testing.slide (right): https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newcode49 2014/testing.slide:49: t.Skip("this doesn't work on ARM") why not give ...
6 years, 5 months ago (2014-08-04 15:56:48 UTC) #14
campoy
LGTM I'd also do: s/` `/`/g 'go test' is semantically one thing, as 'go vet' ...
6 years, 5 months ago (2014-08-04 18:44:14 UTC) #15
r
The spacing looks funny if you write `go vet` rather than `go` `vet` because the ...
6 years, 5 months ago (2014-08-04 19:16:40 UTC) #16
campoy
Should we not use monospace fonts for inline code then? I don't like modifying semantics ...
6 years, 5 months ago (2014-08-04 19:28:31 UTC) #17
bcmills
https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide File 2014/testing.slide (right): https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newcode48 2014/testing.slide:48: if runtime.GOARM == "arm" { s/GOARM/GOARCH/ https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newcode160 2014/testing.slide:160: By ...
6 years, 5 months ago (2014-08-04 19:28:56 UTC) #18
r
There's no semantics being modified here. It's a simple aesthetic choice. As written the presentation ...
6 years, 5 months ago (2014-08-04 19:30:21 UTC) #19
campoy
The generated html is <code>go</code> <code>get</code> instead of <code>go get</code>. In a future I'd like ...
6 years, 5 months ago (2014-08-04 19:41:33 UTC) #20
r
Don't put grey under it and the issue doesn't arise. Let's move on. -rob
6 years, 5 months ago (2014-08-04 19:58:22 UTC) #21
adg
6 years, 5 months ago (2014-08-04 22:47:13 UTC) #22
*** Submitted as
https://code.google.com/p/go/source/detail?r=72552e73e379&repo=talks ***

"Testing Techniques" talk

LGTM=gmlewis, r, campoy
R=dsymonds, crawshaw, gmlewis, r, sameer, bcmills, campoy
CC=golang-codereviews
https://codereview.appspot.com/108170043

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide
File 2014/testing.slide (right):

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:48: if runtime.GOARM == "arm" {
On 2014/08/04 19:28:56, bcmills wrote:
> s/GOARM/GOARCH/

Done.

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:49: t.Skip("this doesn't work on ARM")
On 2014/08/04 15:56:48, r wrote:
> why not give this test a name, to encourage good errors?
> 
> Test80BitFloat doesn't work on ARM

The testing framework puts the test name in the log message.

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:160: By passing a `ResponseRecorder` into an HTTP handler we
can inspect the generated response.
On 2014/08/04 19:28:56, bcmills wrote:
> It might be good to add a sentence or two about when to use which httptest
> helper.
> 
> Server is presumably for testing state (e.g. "what does my handler return if I
> call it with this [method,URL,body]?"), which has a pretty obvious
application.
> 
> I'm less clear on where ResponseRecorder is useful.  It tests interactions,
but
> what kind of interactions can you test with a ResponseRecorder that aren't
> better tested with a Server?  (Perhaps something to do with connection
errors?)
> 
> This really matters a lot in practice: I got a code review the other day that
> was testing some URL redirect stuff on AppEngine, and the author of the code
had
> tested it with ResponseRecorder with a hand-constructed Request.  The author
> hadn't noticed that the URL field of an http.Request is usually only partially
> populated, and the test didn't catch it because the Request wasn't constructed
> by a real Server.  The wrong choice of test helper led to a bug almost
reaching
> production.

The actual talk includes me using these in a real example.
Watch the video if you're curious. (linked from the first slide)

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:184: But "most of the time" means flaky tests.
On 2014/08/04 15:56:48, r wrote:
> "most of the time" doesn't mean "flaky tests".
> 
> But "most of the time" isn't always and flaky tests result.

Done.

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:186: We can use Go's concurrency primitives to make flaky
sleep-driven tests reliable.
On 2014/08/04 15:56:48, r wrote:
> where is this going? seems to stop before anything is told

In the talk I am using these techniques to test my example program. The slides
are only half of the story.

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:193: - bad printf strings,
On 2014/08/04 15:56:48, r wrote:
> bad `printf` formats

Done.

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:198: - copying locks,
On 2014/08/04 15:56:48, r wrote:
> slightly obscure. maybe
> bad use of mutexes

Done.

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:198: - copying locks,
On 2014/08/04 15:56:48, r wrote:
> slightly obscure. maybe
> bad use of mutexes

Done.

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:215: Occasionally you might need to run your tests from
outside the package under test.
On 2014/08/04 19:28:56, bcmills wrote:
> s/need/want/
> 
> For example, you might want the call sites in your Examples to be something
your
> users can paste directly into their code.  Or you might want to check your API
> for stutter, which is easier to do if the test package is imported explicitly.

Done.

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:223: - So the `fmt` tests are in package `fmt_test` and
import both `testing` and `fmt`.
On 2014/08/04 15:56:48, r wrote:
> s/import/can &/

Done.

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:228: Go eschews mocks and fakes in favor of writing code that
takes broad interfaces.
On 2014/08/04 19:28:56, bcmills wrote:
> The connection between interfaces and avoiding test-doubles is not obvious. 
In
> particular, I have seen a lot of attempts to introduce unnecessary interfaces
in
> Go code with the excuse "I need to mock/fake this out for testing", and this
> sentence doesn't address that excuse.
> 
> Go tests tend not to need mocks because the APIs are defined in terms of their
> inputs and outputs, not their interactions.[1]
> 
> Go tests tend not to need fakes because the APIs for real implementations
(e.g.
> bytes.Buffer as a real implementation of io.Reader) don't add much boilerplate
> or overhead.[2]
> 
>
[1]http://googletesting.blogspot.com/2013/03/testing-on-toilet-testing-state-vs.html
>
[2]http://googletesting.blogspot.com/2013/05/testing-on-toilet-dont-overuse-mocks.html

The talk is already done. I sent these slides out more than a month ago. This is
just the final pass before submitting them to the repository.

https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco...
2014/testing.slide:238: (An `*os.File` implements `io.Reader`, as does
`bytes.Buffer` or `strings.Reader`.)
On 2014/08/04 18:44:13, campoy wrote:
> Implements is a bit ambiguous, maybe:
> `*os.File` satisfies `io.Reader`

How is "implements" ambiguous?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b