Issue 108170043: code review 108170043: "Testing Techniques" talk (Closed)
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 #
MessagesTotal messages: 22
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.slid... 2014/testing/testing.slide:43: t.Skipf("this doesn't work on ARM") t.Skip?
Sign in to reply to this message.
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.slid... 2014/testing/testing.slide:176: This means they can access unexported details, as we have already seen. unexported symbols? where have we already seen this? in one of the demos? https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:197: The `"code.google.com/p/go.tools/godoc/vfs"` package specifies a virtual file system interface: Encouraging the use of the godoc vfs just so programs can be tested without disk seems unreasonable. This is a job for a memory file system. There's a reason the godoc vfs hasn't graduated from godoc to a standard library (or even a top-level go.tools package). It's just not general purpose enough. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:247: * Mocks: gomock I'd like to see a note about how you should favor writing tests using real code rather than fakes and mocks unless you see performance problems, or need to test particular calls are made to underlying interfaces.
Sign in to reply to this message.
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.slid... 2014/testing/testing.slide:226: In your tests, you can swap out the `vfs.OS` file system with an alternate implementation, such as the in memory `mapfs` package. Maybe "in-memory"? https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:242: If your code needs more of the file system what is provided godoc's VFS implementation, just make a copy and implement those parts. Maybe "...system than what is provided by godoc's..." ? https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:255: it generates a mock implementation of that interface that behaves as directed: s/it //
Sign in to reply to this message.
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.slid... 2014/testing/testing.slide:12: Go has a built in testing framework. built-in https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:16: This is a complete test file that tests the `strings.Index` function: s/This/Here/ https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:190: - Tests for package `index` are declared in `package` `index_test` and explicitly import package `index`. too complicated. testing package uses fmt, so fmt tests must be outside or we can't test fmt https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:279: * Questions? cover? makes a nice demo
Sign in to reply to this message.
https://codereview.appspot.com/108170043/diff/20001/2014/testing/subprocess/s... File 2014/testing/subprocess/subprocess_test.go (right): https://codereview.appspot.com/108170043/diff/20001/2014/testing/subprocess/s... 2014/testing/subprocess/subprocess_test.go:17: if e, ok := err.(*exec.ExitError); ok && !e.Success() { Invert: if e, ok := err.(*exec.ExitError); !ok || e.Success() { t.Fatalf("process ran with err %v, want exit status 1", err) } https://codereview.appspot.com/108170043/diff/20001/2014/testing/test1/string... File 2014/testing/test1/string_test.go (right): https://codereview.appspot.com/108170043/diff/20001/2014/testing/test1/string... 2014/testing/test1/string_test.go:12: t.Errorf("Index(%q,%q) = %v; want %v", s, sep, want, got) do you want a space between %q,%q to mirror the code? https://codereview.appspot.com/108170043/diff/20001/2014/testing/test2/string... File 2014/testing/test2/string_test.go (right): https://codereview.appspot.com/108170043/diff/20001/2014/testing/test2/string... 2014/testing/test2/string_test.go:10: var tests = []struct { It's common to inline tests in the loop: for _, test := range []struct{ ...fields... }{ ...cases... } { ...body... } https://codereview.appspot.com/108170043/diff/20001/2014/testing/test2/string... 2014/testing/test2/string_test.go:13: out int call this want, like the previous slide? https://codereview.appspot.com/108170043/diff/20001/2014/testing/test2/string... 2014/testing/test2/string_test.go:15: {"", "", 0}, Include {"chicken", "ken", 4} to show that this is just a generalization of the previous slide. https://codereview.appspot.com/108170043/diff/20001/2014/testing/test2/string... 2014/testing/test2/string_test.go:25: t.Errorf("Index(%q,%q) = %v; want %v", test.s, test.sep, actual, test.out) do you want a space between %q,%q to mirror the code? 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.slid... 2014/testing/testing.slide:36: And enabling parallel tests: You haven't mentioned yet that a go test may contain many TestXxx functions. The parallelism here refers to running those functions in parallel, not running the cases of a table-driven test in parallel. People familiar with Go already won't get confused, but people new to Go might. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:190: - Tests for package `index` are declared in `package` `index_test` and explicitly import package `index`. This example makes it seem like indextest and index_test are related. They aren't. Choose something simpler.
Sign in to reply to this message.
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.slid... 2014/testing/testing.slide:12: Go has a built in testing framework. On 2014/06/25 06:15:16, r wrote: > built-in Done. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:16: This is a complete test file that tests the `strings.Index` function: On 2014/06/25 06:15:16, r wrote: > s/This/Here/ Done. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:36: And enabling parallel tests: On 2014/06/25 14:07:16, Sameer at Google wrote: > You haven't mentioned yet that a go test may contain many TestXxx functions. > The parallelism here refers to running those functions in parallel, not running > the cases of a table-driven test in parallel. People familiar with Go already > won't get confused, but people new to Go might. It'll come out in the talk. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:43: t.Skipf("this doesn't work on ARM") On 2014/06/25 02:47:24, dsymonds wrote: > t.Skip? Done. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:176: This means they can access unexported details, as we have already seen. On 2014/06/25 04:40:11, crawshaw1 wrote: > unexported symbols? > > where have we already seen this? in one of the demos? Yep, the previous demos are all about testing implementation. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:190: - Tests for package `index` are declared in `package` `index_test` and explicitly import package `index`. On 2014/06/25 06:15:16, r wrote: > too complicated. > > testing package uses fmt, so fmt tests must be outside or we can't test fmt Much better, thanks. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:190: - Tests for package `index` are declared in `package` `index_test` and explicitly import package `index`. On 2014/06/25 14:07:16, Sameer at Google wrote: > This example makes it seem like indextest and index_test are related. They > aren't. Choose something simpler. Done. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:197: The `"code.google.com/p/go.tools/godoc/vfs"` package specifies a virtual file system interface: On 2014/06/25 04:40:11, crawshaw1 wrote: > Encouraging the use of the godoc vfs just so programs can be tested without disk > seems unreasonable. This is a job for a memory file system. > > There's a reason the godoc vfs hasn't graduated from godoc to a standard library > (or even a top-level go.tools package). It's just not general purpose enough. Added a slide. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:226: In your tests, you can swap out the `vfs.OS` file system with an alternate implementation, such as the in memory `mapfs` package. On 2014/06/25 05:34:03, gmlewis1 wrote: > Maybe "in-memory"? Done. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:242: If your code needs more of the file system what is provided godoc's VFS implementation, just make a copy and implement those parts. On 2014/06/25 05:34:03, gmlewis1 wrote: > Maybe "...system than what is provided by godoc's..." ? Done. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:247: * Mocks: gomock On 2014/06/25 04:40:11, crawshaw1 wrote: > I'd like to see a note about how you should favor writing tests using real code > rather than fakes and mocks unless you see performance problems, or need to test > particular calls are made to underlying interfaces. Done. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:255: it generates a mock implementation of that interface that behaves as directed: On 2014/06/25 05:34:03, gmlewis1 wrote: > s/it // Done. https://codereview.appspot.com/108170043/diff/20001/2014/testing/testing.slid... 2014/testing/testing.slide:279: * Questions? On 2014/06/25 06:15:16, r wrote: > cover? makes a nice demo Ah, thanks for reminding me. I had intended to put it in as part of my demo. Added slides.
Sign in to reply to this message.
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.slid... 2014/testing/testing.slide:42: if runtime.GOARM == "arm" { runtime.GOARCH https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... 2014/testing/testing.slide:290: * Mocks: gomock Gomock is overused. Could we mention when (not) to use it instead of just that it exists? (Once folks learn about mocking, they tend to start testing interactions even though their API is specified in terms of behaviors instead.) At the very least, could we find a more realistic/appropriate example?
Sign in to reply to this message.
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.slid... 2014/testing/testing.slide:36: And enabling parallel tests: i agree with dvyukov that this is a subtle point to explain well. maybe better just to drop it. https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... 2014/testing/testing.slide:40: And controlling whether a test runs at all: testing.Short is nice; might use it instead of Parallel. https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... 2014/testing/testing.slide:183: - bad printf strings, bad `Printf` formats https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... 2014/testing/testing.slide:236: * Fakes too much about fakes in my opinion. they're very rare (see previous slide). https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... 2014/testing/testing.slide:298: generates a mock implementation of that interface that behaves as directed: it's not a mock implementation (although some would call it that). it's a recording implementation that checks superficial properties of the client. it also doesn't behave as directed, except in the way the test tells it to behave. they enshrine fragile, pointless properties in short, i don't like "mocks" and think they are bad, and shouldn't be singled out.
Sign in to reply to this message.
Thanks for serendipitously backing me on mocks, bcmills. -rob On Wed, Jun 25, 2014 at 10:12 AM, <r@golang.org> wrote: > > 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.slid... > > 2014/testing/testing.slide:36: And enabling parallel tests: > i agree with dvyukov that this is a subtle point to explain well. maybe > better just to drop it. > > https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... > 2014/testing/testing.slide:40: And controlling whether a test runs at > all: > testing.Short is nice; might use it instead of Parallel. > > https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... > 2014/testing/testing.slide:183: - bad printf strings, > bad `Printf` formats > > https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... > 2014/testing/testing.slide:236: * Fakes > too much about fakes in my opinion. they're very rare (see previous > slide). > > https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... > 2014/testing/testing.slide:298: generates a mock implementation of that > > interface that behaves as directed: > it's not a mock implementation (although some would call it that). it's > a recording implementation that checks superficial properties of the > client. > > it also doesn't behave as directed, except in the way the test tells it > to behave. they enshrine fragile, pointless properties > > in short, i don't like "mocks" and think they are bad, and shouldn't be > singled out. > > https://codereview.appspot.com/108170043/
Sign in to reply to this message.
On 2014/06/25 17:13:04, r wrote: > Thanks for serendipitously backing me on mocks, bcmills. > > -rob > > > On Wed, Jun 25, 2014 at 10:12 AM, <mailto:r@golang.org> wrote: > > > > > 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.slid... > > > > 2014/testing/testing.slide:36: And enabling parallel tests: > > i agree with dvyukov that this is a subtle point to explain well. maybe > > better just to drop it. > > > > > https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... > > 2014/testing/testing.slide:40: And controlling whether a test runs at > > all: > > testing.Short is nice; might use it instead of Parallel. > > > > > https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... > > 2014/testing/testing.slide:183: - bad printf strings, > > bad `Printf` formats > > > > > https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... > > 2014/testing/testing.slide:236: * Fakes > > too much about fakes in my opinion. they're very rare (see previous > > slide). > > > > > https://codereview.appspot.com/108170043/diff/60001/2014/testing/testing.slid... > > 2014/testing/testing.slide:298: generates a mock implementation of that > > > > interface that behaves as directed: > > it's not a mock implementation (although some would call it that). it's > > a recording implementation that checks superficial properties of the > > client. > > > > it also doesn't behave as directed, except in the way the test tells it > > to behave. they enshrine fragile, pointless properties > > > > in short, i don't like "mocks" and think they are bad, and shouldn't be > > singled out. > > > > https://codereview.appspot.com/108170043/ was this ever submitted?
Sign in to reply to this message.
On 12 July 2014 09:46, <campoy@google.com> wrote: > was this ever submitted? No. These are the slides for the live demo only; I need to add more context for them to stand alone.
Sign in to reply to this message.
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 this change to https://code.google.com/p/go.talks
Sign in to reply to this message.
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 (cc: > mailto:golang-codereviews@googlegroups.com), > > I'd like you to review this change to > https://code.google.com/p/go.talks LGTM
Sign in to reply to this message.
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#newco... 2014/testing.slide:49: t.Skip("this doesn't work on ARM") why not give this test a name, to encourage good errors? Test80BitFloat doesn't work on ARM https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco... 2014/testing.slide:184: But "most of the time" means flaky tests. "most of the time" doesn't mean "flaky tests". But "most of the time" isn't always and flaky tests result. 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. where is this going? seems to stop before anything is told https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco... 2014/testing.slide:193: - bad printf strings, bad `printf` formats https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco... 2014/testing.slide:198: - copying locks, slightly obscure. maybe bad use of mutexes 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`. s/import/can &/
Sign in to reply to this message.
LGTM I'd also do: s/` `/`/g 'go test' is semantically one thing, as 'go vet' etc. 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:20: It is provided by the `testing` package and the `go` `test` command. remove the space between `go` and `test`. 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`.) Implements is a bit ambiguous, maybe: `*os.File` satisfies `io.Reader`
Sign in to reply to this message.
The spacing looks funny if you write `go vet` rather than `go` `vet` because the constant-width space is too wide. -rob On Mon, Aug 4, 2014 at 11:44 AM, <campoy@google.com> wrote: > LGTM > > I'd also do: s/` `/`/g > 'go test' is semantically one thing, as 'go vet' etc. > > > > 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:20: It is provided by the `testing` package and the > `go` `test` command. > remove the space between `go` and `test`. > > 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`.) > Implements is a bit ambiguous, maybe: > `*os.File` satisfies `io.Reader` > > https://codereview.appspot.com/108170043/
Sign in to reply to this message.
Should we not use monospace fonts for inline code then? I don't like modifying semantics for better looks. And I imagine a future where present provides custom css styles. On Mon, Aug 4, 2014 at 12:16 PM, Rob Pike <r@golang.org> wrote: > The spacing looks funny if you write `go vet` rather than `go` `vet` > because the constant-width space is too wide. > > -rob > > > On Mon, Aug 4, 2014 at 11:44 AM, <campoy@google.com> wrote: > > LGTM > > > > I'd also do: s/` `/`/g > > 'go test' is semantically one thing, as 'go vet' etc. > > > > > > > > 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:20: It is provided by the `testing` package and the > > `go` `test` command. > > remove the space between `go` and `test`. > > > > > 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`.) > > Implements is a bit ambiguous, maybe: > > `*os.File` satisfies `io.Reader` > > > > https://codereview.appspot.com/108170043/ >
Sign in to reply to this message.
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" { s/GOARM/GOARCH/ 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. 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. 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. 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. https://codereview.appspot.com/108170043/diff/140001/2014/testing.slide#newco... 2014/testing.slide:219: This is done to break dependency cycles. For example: "This can break dependency cycles." ("is done" implies that breaking cycles is the only reason to use a separate package, which I would dispute.) 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. 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
Sign in to reply to this message.
There's no semantics being modified here. It's a simple aesthetic choice. As written the presentation looks better. -rob On Mon, Aug 4, 2014 at 12:28 PM, Francesc Campoy Flores <campoy@google.com> wrote: > Should we not use monospace fonts for inline code then? > > I don't like modifying semantics for better looks. > And I imagine a future where present provides custom css styles. > > > On Mon, Aug 4, 2014 at 12:16 PM, Rob Pike <r@golang.org> wrote: >> >> The spacing looks funny if you write `go vet` rather than `go` `vet` >> because the constant-width space is too wide. >> >> -rob >> >> >> On Mon, Aug 4, 2014 at 11:44 AM, <campoy@google.com> wrote: >> > LGTM >> > >> > I'd also do: s/` `/`/g >> > 'go test' is semantically one thing, as 'go vet' etc. >> > >> > >> > >> > 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:20: It is provided by the `testing` package and the >> > `go` `test` command. >> > remove the space between `go` and `test`. >> > >> > >> > 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`.) >> > Implements is a bit ambiguous, maybe: >> > `*os.File` satisfies `io.Reader` >> > >> > https://codereview.appspot.com/108170043/ > >
Sign in to reply to this message.
The generated html is <code>go</code> <code>get</code> instead of <code>go get</code>. In a future I'd like to provide different css styles for present. I'd like to avoid this: [image: Inline image 1] On Mon, Aug 4, 2014 at 12:29 PM, Rob Pike <r@golang.org> wrote: > There's no semantics being modified here. It's a simple aesthetic > choice. As written the presentation looks better. > > -rob > > > On Mon, Aug 4, 2014 at 12:28 PM, Francesc Campoy Flores > <campoy@google.com> wrote: > > Should we not use monospace fonts for inline code then? > > > > I don't like modifying semantics for better looks. > > And I imagine a future where present provides custom css styles. > > > > > > On Mon, Aug 4, 2014 at 12:16 PM, Rob Pike <r@golang.org> wrote: > >> > >> The spacing looks funny if you write `go vet` rather than `go` `vet` > >> because the constant-width space is too wide. > >> > >> -rob > >> > >> > >> On Mon, Aug 4, 2014 at 11:44 AM, <campoy@google.com> wrote: > >> > LGTM > >> > > >> > I'd also do: s/` `/`/g > >> > 'go test' is semantically one thing, as 'go vet' etc. > >> > > >> > > >> > > >> > > 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:20: It is provided by the `testing` package and the > >> > `go` `test` command. > >> > remove the space between `go` and `test`. > >> > > >> > > >> > > 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`.) > >> > Implements is a bit ambiguous, maybe: > >> > `*os.File` satisfies `io.Reader` > >> > > >> > https://codereview.appspot.com/108170043/ > > > > >
Sign in to reply to this message.
Don't put grey under it and the issue doesn't arise. Let's move on. -rob
Sign in to reply to this message.
*** 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.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
