Increase consistency of math tutorial with other tutorials #266

pamelafox · 2019-10-13T22:15:05Z

Thanks for the tutorial, @gypsydave5!

I noticed various inconsistencies with the math tutorial as compared to the other tutorials in terms of test output display and package organization. Specifically:

Instead of displaying entire test display output, I change it to only show the single line and report ("clockface_test.go:17: Got {0 0}, wanted {150 60}"). I could see the motivation for including the line above that line, since it includes the test name, but in my opinion, the additional lines underneath just add noise and indentation that decrease the tutorial readability.
Some of the tests seem to assume they're in a separate package from the functionality they're testing (clockface.SecondHand). I change them to assume they're in same package.
I trimmed the output for tests passing output. The other tutorials don't display tests passing output, so I wasn't sure what would be consistent, I'm not sure this change of mine is helpful/accurate.

I'll add in-line comments as well.

Trims test output, assumes a single test file, other tweaks.

pamelafox · 2019-10-13T22:17:21Z

Also, overall comment: this tutorial uses the phrase "acceptance test" and seems (in the repo) to place some tests in clock_acceptance_test.go and the rest in clock_test.go. I found that confusing as it wasn't clear what we were putting where, and would appreciate a formal introduction to "acceptance test" (the way you're using it) and guidance on file structure (if you think we should indeed split the test files).

pamelafox · 2019-10-13T22:17:50Z

math.md

@@ -95,20 +95,18 @@ focus on testing something that creates them.
 So my first test looks like this:

 ```go
-package clockface_test
+package clockface


This is an example of a test file that seemed to think it was testing functionality in a different package. I changed the package name and removed "clockface" in front of the functions.

pamelafox · 2019-10-13T22:18:29Z

math.md

 ```

 ### Write enough code to make it pass

-When we get the expected failure, we can fill in the return value of `HandsAt`:
+When we get the expected failure, we can fill in the return value of `SecondHand`:


pamelafox · 2019-10-13T22:20:29Z

math.md

@@ -183,7 +174,7 @@ Behold, a passing test.

 ```
 PASS
-ok  	github.com/gypsydave5/learn-go-with-tests/math/v1/clockface	0.006s
+ok  	clockface	0.006s


This is an example of simplifying the passing tests output. I got confused at first by all the github.com/gypsydave5 references (as it was also in the imports for a few files), so I thought removing all the references might reduce confusion. However, I realize now that Go always outputs the full path so this simplification may suffer accuracy.

pamelafox · 2019-10-13T22:21:40Z

math.md

 hard work for us.  Even better, there's an online version at
 [https://www.onlinetool.io/xmltogo/](https://www.onlinetool.io/xmltogo/). Just
-paste the SVG from the top of the file into one box and - bam
- out pops:
+paste the SVG from the top of the file into one box and - bam! - out pops:


When the hyphen is on a new line , it makes a bulleted list instead. Fixed this in a few places.

pamelafox · 2019-10-13T22:23:26Z

math.md

@@ -952,14 +903,16 @@ type Svg struct {
 ```

 We could make adjustments to this if we needed to (like changing the name of the
-struct to `SVG`) but it's definitely good enough to start us off.
+struct to `SVG`) but it's definitely good enough to start us off. Paste that struct into the same file as the clockface functions.


Actually, looks like you wanted this in the test file, so I should edit this! TODO.

pamelafox · 2019-10-13T22:23:51Z

math.md

@@ -1020,7 +957,7 @@ func secondHand(w io.Writer, t time.Time) {
 	p = Point{p.X * secondHandLength, p.Y * secondHandLength} // scale
 	p = Point{p.X, -p.Y}                                      // flip
 	p = Point{p.X + clockCentreX, p.Y + clockCentreY}         // translate
-	fmt.Fprintf(w, `<line x1="150" y1="150" x2="%.3f" y2="%.3f" style="fill:none;stroke:#f00;stroke-width:3px;"/>`, p.X, p.Y)
+	fmt.Fprintf(w, `<line x1="150" y1="150" x2="%f" y2="%f" style="fill:none;stroke:#f00;stroke-width:3px;"/>`, p.X, p.Y)


This code had a spoiler alert

good spot 👍

pamelafox · 2019-10-13T22:24:18Z

math.md

@@ -1118,8 +1042,8 @@ tests, to sharpen everything up.
 type SVG struct {
 	XMLName xml.Name `xml:"svg"`
 	Xmlns   string   `xml:"xmlns,attr"`
-	Width   float64  `xml:"width,attr"`
-	Height  float64  `xml:"height,attr"`
+	Width   string  `xml:"width,attr"`


width is 100%, so these needed to stay strings, else it failed to parse for me

pamelafox · 2019-10-13T22:25:29Z

math.md

 But the proof of the pudding is in the eating - if we now compile and run our
 `clockface` program, we should see something like

-![a clock with second and minute hands](math/v9/clockface/clockface/clock.svg)
+![a clock with only a second hand](math/v9/clockface/clockface/clock.svg)


Not sure why this reverted. TODO

pamelafox · 2019-10-13T22:26:34Z

math.md

 ```

 ### Write enough code to make it pass

-And we can now make our final adjustments to `svgWriter.go`
+And we can now make our final adjustments to the SVG writing constants and functions:


The tutorial never mentioned a svgWriter.go file before. I'd suggest it mentioning it earlier, if the goal is for us to split the functions like that.

pamelafox · 2019-10-13T22:26:47Z

math.md

@@ -2122,7 +2010,7 @@ document how it works.
 > a C program.
 >			-- Henry Spencer, in _The Art of Unix Programming_

-In [my final take on this program](math/vFinal/clockface), I've made the
+In [my final take on this program](https://github.com/gypsydave5/learn-go-with-tests/tree/master/math/vFinal/clockface), I've made the


Current URL 404s

quii · 2019-10-14T06:18:33Z

Wow, thanks for all this effort @pamelafox I will try and find some time to look at this over the coming week

gypsydave5 · 2019-10-17T13:38:01Z

Also, overall comment: this tutorial uses the phrase "acceptance test" and seems (in the repo) to place some tests in clock_acceptance_test.go and the rest in clock_test.go. I found that confusing as it wasn't clear what we were putting where, and would appreciate a formal introduction to "acceptance test" (the way you're using it) and guidance on file structure (if you think we should indeed split the test files).

This is fair, if I've not introduced this idea it's because I thought @quii had already done it for me... the basic principle is that an 'acceptance' test is testing the behaviour you want to achieve from the point of view of a user (to be 'accepted' by them), whereas 'unit' tests are usually working at a lower level than the surface of the application. These are not well defined terms though!

I'll try and work something up to explain this division.

pamelafox · 2019-10-22T21:57:16Z

@gypsydave5 The other type of test I've seen introduced are integration tests, in the web application tutorial. That would still come after your SVG tutorial, though, and an integration test isn't the same as acceptance test. (They might happen to be the same in this case, but wouldn't always be). Here's where integration tests are introduced: https://quii.gitbook.io/learn-go-with-tests/build-an-application/http-server#integration-tests
You could do a similar treatment for Acceptance tests in your tutorial.

quii · 2020-12-23T08:07:20Z

Just commenting to remind myself of the shame of this. I am going to try and get someone (@gypsydave5 cough) to pair with me on this next year to get it merged. Really sorry @pamelafox

pamelafox · 2021-01-26T03:21:29Z

No worries, I'm similarly a year behind on responding to OSS PRs. :)

(and a few by me) This is easier than me trying to work through the diff which I failed at for the last twelve months. The original PR is quii#266 Apologies that it took so long. Co-authored-by: Pamela Fox <pamela.fox@gmail.com>

gypsydave5 · 2021-06-11T12:28:13Z

OK - hey @pamelafox and @quii - I've gone through the changes in a separate PR one by one and added in all the ones I agree with - which is most of them! - along with a few other changes I've wanted to make.

@pamelafox I've tagged you as a co-author, is this OK?

(and a few by me) This is easier than me trying to work through the diff which I failed at for the last twelve months. The original PR is quii#266 Apologies that it took so long. Co-authored-by: Pamela Fox <pamela.fox@gmail.com>

pamelafox · 2021-06-11T14:34:45Z

Sounds good, I'll close this one!

(and a few by me) This is easier than me trying to work through the diff which I failed at for the last twelve months. The original PR is #266 Apologies that it took so long. Co-authored-by: Pamela Fox <pamela.fox@gmail.com> Co-authored-by: Pamela Fox <pamela.fox@gmail.com>

(and a few by me) This is easier than me trying to work through the diff which I failed at for the last twelve months. The original PR is quii#266 Apologies that it took so long. Co-authored-by: Pamela Fox <pamela.fox@gmail.com> Co-authored-by: Pamela Fox <pamela.fox@gmail.com>

Increase consistency of math tutorial with other tutorials

322f8ff

Trims test output, assumes a single test file, other tweaks.

pamelafox commented Oct 13, 2019

View reviewed changes

voelzmo mentioned this pull request May 18, 2021

'math' chapter is hard to follow #457

Closed

11 tasks

gypsydave5 mentioned this pull request Jun 11, 2021

Changes authored by Pamela Fox #466

Closed

pamelafox closed this Jun 11, 2021

gypsydave5 mentioned this pull request Jun 11, 2021

Changes authored by Pamela Fox #467

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase consistency of math tutorial with other tutorials #266

Increase consistency of math tutorial with other tutorials #266

pamelafox commented Oct 13, 2019

pamelafox commented Oct 13, 2019

pamelafox Oct 13, 2019

pamelafox Oct 13, 2019

pamelafox Oct 13, 2019

pamelafox Oct 13, 2019

pamelafox Oct 13, 2019

pamelafox Oct 13, 2019

gypsydave5 Oct 17, 2019

pamelafox Oct 13, 2019

pamelafox Oct 13, 2019

pamelafox Oct 13, 2019

pamelafox Oct 13, 2019

quii commented Oct 14, 2019

gypsydave5 commented Oct 17, 2019

pamelafox commented Oct 22, 2019

quii commented Dec 23, 2020

pamelafox commented Jan 26, 2021

gypsydave5 commented Jun 11, 2021

pamelafox commented Jun 11, 2021

Increase consistency of math tutorial with other tutorials #266

Increase consistency of math tutorial with other tutorials #266

Conversation

pamelafox commented Oct 13, 2019

pamelafox commented Oct 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quii commented Oct 14, 2019

gypsydave5 commented Oct 17, 2019

pamelafox commented Oct 22, 2019

quii commented Dec 23, 2020

pamelafox commented Jan 26, 2021

gypsydave5 commented Jun 11, 2021

pamelafox commented Jun 11, 2021