Bug 73956 – Rewrite the test results server in Go

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Ojan Vafai]

Rewrite the test results server in Go

[Ojan Vafai]

Created attachment 118126 [details]
Patch

[Ojan Vafai]

Adam, you mind doing an informal review of the Go code here?

[Adam Langley]

Comment on attachment 118126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118126&action=review

I'm afraid that this very much feels like Python code written in Go. There's not a comment, and there's lots of python functions reimplemented in Go.

I'm pretty sure that json.go should be using Marshal, but I can't understand what it's trying to do. Also, I feel that the form parsing functions may want to use reflection, although that depends on how many more are expected over time.

> Tools/TestsDashboardServer/base/base.go:4
> +	"io"

Sort includes

> Tools/TestsDashboardServer/base/base.go:10
> +const READ_GRANULARITY = 4096

This will be an exported symbol, are you sure you want that? Typically it would be called readGranularity.

> Tools/TestsDashboardServer/base/base.go:44
> +	for {

Isn't this ioutil.ReadAll?

> Tools/TestsDashboardServer/base/base.go:57
> +func WriteBytes(to io.Writer, from io.Reader) os.Error {

io.Copy?

> Tools/TestsDashboardServer/base/base_test.go:9
> +func assertEqual(t *testing.T, actual interface{}, expected interface{}) {

These generic functions are specifically not implemented in the testing package in order to avoid the poor error messages that result.

> Tools/TestsDashboardServer/base/http.go:4
> +	"fmt"

sort imports.

> Tools/TestsDashboardServer/base/http.go:18
> +const MAX_MEMORY_FOR_FORM = 10 << 20

I don't think that any of these are intended to be exported.

> Tools/TestsDashboardServer/base/http.go:19
> +const MASTER = "master"

use a const block:

const (
   foo = 1
   bar = 2
)

> Tools/TestsDashboardServer/base/http.go:51
> +		fmt.Fprintf(writer, str)

Do you really want to interpret printf codes in the string? Maybe io.WriteString(writer, str)

> Tools/TestsDashboardServer/base/http.go:65
> +	fmt.Fprint(writer, "\n"+string(debug.Stack()))

Likewise about io.WriteString here and elsewhere.

> Tools/TestsDashboardServer/base/http.go:88
> +		number, err := strconv.Atoi(queryParameters.Get(LIMIT))

if number, err := strconv.Atoi(...); err == nil {
  limit = number
}

> Tools/TestsDashboardServer/base/http.go:156
> +	isPureJson := regexp.MustCompile("^{.*}$").Match(jsonBytes)

This would appear to be a very inefficient way of doing this :)

[Ojan Vafai]

(In reply to comment #3)
> I'm afraid that this very much feels like Python code written in Go. There's not a comment, and there's lots of python functions reimplemented in Go.

I meant to mention, this is the first Go code I've ever written, so feel free to be harsh in your feedback. The places where I'm doing something dumb are likely places I don't know any better and not intentional. :)

If there are specific functions or high-level design I could change to make this more idiomatic Go, I'd be happy to.

As far as the lack of comments, I've followed WebKit style of only writing comments that explain "why", not "what".

> I'm pretty sure that json.go should be using Marshal, but I can't understand what it's trying to do.

Yup, I couldn't come up with a struct for this data. There are two problems I couldn't really work around:
1. Some keys are not fixed (e.g. builder and test names are keys)
2. One part of the JSON is recursive.

The data looks like this:
{
"version": 4,
"MyBuilderName": {
    "builds": [3, 2, 1],
    "tests": {
        "foo": {
            "001.html": {"results": [[1, "I"],[1, "F"]], "times": [[1, 20],[1, 0]]},
            "002.html": {"results": [[1, "I"],[1, "F"]], "times": [[1, 20],[1, 0]]}
        },
        "003.html": {"results": [[1, "T"],[1, "F"]], "times": [[1, 10],[1, 0]]}
    }
}
}

"MyBuilderName" changes and the subtree under tests is a recursive data structure where the leaves are always these results/times pairs.

I do know the builder name before doing the decoding/encoding. I suppose I could use reflection to change the name of that field to the builder name before decoding/encoding.

> Also, I feel that the form parsing functions may want to use reflection, although that depends on how many more are expected over time.

I don't expect there to be any new ones. This codebase is very unlikely to grow much beyond what's there now. Famous last words of course. :) How would I use reflection to do this?

I guess I would do something like:
type UploadFormData struct {
	Builder  string "builder"
	Master   string "master"
}
and then use the tag to lookup the field in the form?

[Ojan Vafai]

Created attachment 118275 [details]
Patch

[Alexey Proskuryakov]

Could you please send an e-mail to webkit-dev before introducing a new programming language to WebKit repository?

[Ojan Vafai]

Comment on attachment 118126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118126&action=review

>> Tools/TestsDashboardServer/base/base.go:4
>> +	"io"
> 
> Sort includes

Whoops. I figured gofmt would complain if I got this wrong.

>> Tools/TestsDashboardServer/base/base.go:44
>> +	for {
> 
> Isn't this ioutil.ReadAll?

Sigh. Yes.

>> Tools/TestsDashboardServer/base/base.go:57
>> +func WriteBytes(to io.Writer, from io.Reader) os.Error {
> 
> io.Copy?

Also, lesigh. I think I wrote these before I got comfortable navigating the Go documentation.

>> Tools/TestsDashboardServer/base/base_test.go:9
>> +func assertEqual(t *testing.T, actual interface{}, expected interface{}) {
> 
> These generic functions are specifically not implemented in the testing package in order to avoid the poor error messages that result.

I prefer the conciseness of these functions. I added a "message" argument instead to give better error messages. To my eye, they're comparable quality to the ones in the standard go packages.

>> Tools/TestsDashboardServer/base/http.go:4
>> +	"fmt"
> 
> sort imports.

Apparently "o" comes after "s" in my head. :(

>> Tools/TestsDashboardServer/base/http.go:18
>> +const MAX_MEMORY_FOR_FORM = 10 << 20
> 
> I don't think that any of these are intended to be exported.

fixed

>> Tools/TestsDashboardServer/base/http.go:19
>> +const MASTER = "master"
> 
> use a const block:
> 
> const (
>    foo = 1
>    bar = 2
> )

much better

>> Tools/TestsDashboardServer/base/http.go:51
>> +		fmt.Fprintf(writer, str)
> 
> Do you really want to interpret printf codes in the string? Maybe io.WriteString(writer, str)

No. Was just cargo culting the appengine Go examples. Fixed.

>> Tools/TestsDashboardServer/base/http.go:156
>> +	isPureJson := regexp.MustCompile("^{.*}$").Match(jsonBytes)
> 
> This would appear to be a very inefficient way of doing this :)

Sigh. Clearly wasn't thinking straight when I wrote this.

[Adam Langley]

Comment on attachment 118275 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118275&action=review

LGTM

> Tools/TestsDashboardServer/base/base.go:12
> +	var message string

You can save the default case

message := ""
switch value := value.(type) {
case string:
  message = value + "\n"
case os.Error:
  message = value.String + "\n"
}
return ...

> Tools/TestsDashboardServer/base/http.go:14
> +// Picked a very large number. 10MB should be plenty.

this comment should probably be moved above |maxMemoryForForm|

> Tools/TestsDashboardServer/base/http.go:60
> +func Serve403(writer http.ResponseWriter, html ...string) os.Error {

Except for Serve500, these functions don't really justify their existence.

> Tools/TestsDashboardServer/base/http.go:71
> +	return MultiError(serveHtmlErr, debugStackErr)

Normally this would be written:

if err := serveHtml(writer, 500, html...); err != {
  return err
}
return io.WriteString(...)

(If a write fails then further writes are unlikely to work.)

> Tools/TestsDashboardServer/base/http.go:84
> +		if matched, _ := regexp.MatchString("^[A-Za-z0-9_]+$", callback); !matched {

This works, but I would normally expect strings.IndexFunc to save on regexp compilation each time.

> Tools/TestsDashboardServer/base/http.go:91
> +		if number, err := strconv.Atoi(queryParameters.Get(limitParameter)); err == nil {

Is it ok that limit may be very large, or negative?

> Tools/TestsDashboardServer/base/http.go:109
> +func ParseUploadFormData(request *http.Request) (UploadFormData, os.Error) {

you should name the return values here: (formData UploadFormData, os.Error)

> Tools/TestsDashboardServer/base/http.go:110
> +	var formData UploadFormData

remove this

> Tools/TestsDashboardServer/base/http.go:111
> +	if err := request.ParseMultipartForm(maxMemoryForForm); err != nil {

then do:

if err = equest.ParseMultipartForm(maxMemoryForForm); err != nil {
  return
}

err won't shadow the return value because it's not a :=.

> Tools/TestsDashboardServer/base/http.go:115
> +	form := request.MultipartForm

if formData.form = request.MultipartForm; formData.form == nil {
  err = Error("missing post data")
}

> Tools/TestsDashboardServer/base/http.go:120
> +	builder := form.Value[builderParameter]

ditto for these. You can cover x == nil || x == "" with len(x) == 0 because len(nil) == 0.

> Tools/TestsDashboardServer/base/http.go:140
> +	formData = UploadFormData{

then you can remove this and just "return". If err got set to a non-nil value then it'll get returned.

> Tools/TestsDashboardServer/base/http.go:157
> +	isPureJson := jsonBytes[0] == '{' && jsonBytes[len(jsonBytes)-1] == '}'

add a len(jsonBytes) > 1 at the beginning.

> Tools/TestsDashboardServer/base/http.go:160
> +			jsonBytes = StripJsonpCallback(jsonBytes)

I can't see that StripJsonpCallback is called from anywhere else (save tests) so it should probably not be exported.

> Tools/TestsDashboardServer/base/http.go:162
> +		_, err := io.WriteString(writer, string(jsonBytes))

io.WriteString(x, string(y)) ==> x.Write(y)

> Tools/TestsDashboardServer/base/http.go:175
> +	if _, err := io.WriteString(writer, string(jsonBytes)); err != nil {

writer.Write(jsonBytes)

> Tools/TestsDashboardServer/base/http_test.go:20
> +type mockHttpResponseWriter struct {

It looks like you want to embed the bytes.Buffer:

type mockHttpResponseWriter struct {
  bytes.Buffer
  header http.Header
  status int
}

then you don't need to forward Write() etc. Also, the zero value for a bytes.Buffer should work fine as an empty Buffer.

Once you've done this, you can create the http.Header on demand in Header() and remove newMockHttpResponseWriter

> Tools/TestsDashboardServer/base/json.go:14
> +const TESTS_KEY = "tests"

this could be a "const (" block and the CAPITAL_FORM means that they're all exported. Probably doesn't matter, but it's non-standard.

(I would usually expect "times", "results" etc as the names, but it's up to you.)

> Tools/TestsDashboardServer/base/json.go:48
> +type testJson struct {

this could just be:

type testJson interface{}

> Tools/TestsDashboardServer/base/json.go:63
> +		fs = append(fs, testJson{val: v})

If you replace testJson as above, then this becomes "testJson(v)"

> Tools/TestsDashboardServer/base/json.go:76
> +	json = regexp.MustCompile("^[A-Za-z0-9_]+[(]").ReplaceAll(json, emptyByte)

You can precompile this, and other regexps by putting:

var reJSONPPrelude = regexp.MustCompile("xyz")

at the top level and then using the variable.

> Tools/TestsDashboardServer/base/json.go:80
> +func removeResultsAndTimes(tests testJson) {

I'm not sure why you switched from defining methods on testJson.

> Tools/TestsDashboardServer/base/json.go:92
> +	for key, _ := range builderData {

I don't think you need the ", _"

> Tools/TestsDashboardServer/base/json.go:101
> +func TestFileJson(reader io.Reader) (ResultsJson, os.Error) {

func TestFileJson(reader io.Reader) (out ResultsJson, err os.Error) {
  err = json.NewDecoder(reader).Decode(&out)
  return
}

> Tools/TestsDashboardServer/base/json.go:173
> +	allTests := make(map[string]bool)

var allTests []string
for testName := range aggregateTests {
  allTests = append(allTests, testName)
}
for testName := range ... {
  ..
}

> Tools/TestsDashboardServer/base/json.go:261
> +	buffer := bytes.NewBuffer(make([]byte, 0))

I think the zero value of bytes.Buffer suffices here:

buffer := new(bytes.Buffer)

> Tools/TestsDashboardServer/base/json_test.go:15
> +		fmt.Printf(err.String())

t.Fatal(err.String) ?

> Tools/TestsDashboardServer/base/json_test.go:24
> +		fmt.Printf(err.String())

t.Fatal?

> Tools/TestsDashboardServer/base/json_test.go:31
> +func roundTripJson(t *testing.T, json string) string {

removeJSONWhitespace?

> Tools/TestsDashboardServer/base/json_test.go:36
> +	stripped := string(StripJsonpCallback([]byte("CALLBACK([content])")))

The number of test cases here is pretty small, but as example, this sort of thing would usually be written as:

var stripJSONPTests = []struct{
  in, out string
}{
  {"CALLBACK([content])", "[content]"},
  ...
}

func TestStripJsonpCallback(t *testing.T) {
  for i, test := range stripJSONPTests {
    if actual := StripJsonpCallback(test.in); actual != test.out {
      t.Errorf("#%d: got '%s', want '%s'", i, actual, test.out)
    }
  }
}

> Tools/TestsDashboardServer/base/json_test.go:61
> +	buffer := bytes.NewBufferString("")

buffer := new(bytes.Buffer)

> Tools/TestsDashboardServer/run/run.go:76
> +func serve500(context appengine.Context, writer http.ResponseWriter, html ...string) {

This is the same function three times.

[Ojan Vafai]

There's resistance to adding Go to the WebKit repository and the benefits of this rewrite aren't strong enough for me to push for it.