7
\$\begingroup\$

I have a short test to validate that all committed files in the repo are formatted. I'm new to go so not sure what the best wat to do it is.

The error handling, in particular, is rather gruesome as it takes up roughly half of the lines.

package pack

import (
    "os"
    "os/exec"
    "path/filepath"
    "testing"
)

func TestFormatting(t *testing.T) {
    var commandArgs []string
    commandArgs = append(commandArgs, "-l")
    root := "../.."
    err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
        if filepath.Ext(path) != ".go" {
            return nil
        }
        absPath, e := filepath.Abs(path)
        if e != nil {
            return e
        }
        commandArgs = append(commandArgs, absPath)
        return nil
    })
    if err != nil {
        t.Logf(err.Error())
        t.Fail()
    }

    out, err := exec.Command("gofmt", commandArgs...).Output()
    if err != nil {
        t.Logf(err.Error())
        t.Fail()
    }
    output := string(out)
    if output != "" {
        t.Logf("Testing Formatting")
        t.Logf(output)
        t.Fail()
    }
}

How can it be improved?

\$\endgroup\$
1
  • \$\begingroup\$ I usually have a small utility function func fatalIfErr(testing.BT, error) that calls Fatal if err is not not. So I can do fatalIfErr(t, err) to avoid repeating myself. \$\endgroup\$
    – Eyal
    Dec 9, 2019 at 14:39

1 Answer 1

5
\$\begingroup\$

a short test to validate that all committed files in the repo are formatted.


Here are some notes I made while reading your code.


For a real-world code review, code should be correct, maintainable, reasonably efficient, and, most importantly, readable.

Writing code is a process of stepwise refinement.

Define the problem with an outline of a solution.

// checkGoFmt checks that Go source files have been formatted with go fmt or gofmt.
// checkGoFmt returns a newline separated list of files
// whose formatting differs from gofmt's and any error.
func checkGoFmt(root string) (string, error) {
    // Command gofmt
    // https://golang.org/cmd/gofmt/
    // $ gofmt -help
    // usage: gofmt [flags] [path ...]
    //  -l  list files whose formatting differs from gofmt's
    // $
}

Read the documentation for the gofmt command.

Write some code to construct and run the gofmt command. The goFiles function is a stub.

func goFiles(root string) ([]string, error) {
    var files []string
    return files, nil
}

// checkGoFmt checks that Go source files have been formatted with go fmt or gofmt.
// checkGoFmt returns a newline separated list of files
// whose formatting differs from gofmt's and any error.
func checkGoFmt(root string) (string, error) {
    // Command gofmt
    // https://golang.org/cmd/gofmt/
    // $ gofmt -help
    // usage: gofmt [flags] [path ...]
    //  -l  list files whose formatting differs from gofmt's
    // $

    files, err := goFiles(root)
    if err != nil {
        return "", err
    }
    if len(files) == 0 {
        return "", nil
    }

    args := make([]string, 0, 1+len(files))
    args = append(args, "-l")
    args = append(args, files...)
    out, err := exec.Command("gofmt", args...).Output()
    return string(out), err
}

Pay attention to program structure. The specific implementation of goFiles is hidden from checkGoFmt. If the end result is not as expected, start by looking at the list of files returned from goFiles. Consider writing a test for the goFiles function. checkGoFmt can be called from a testing package function or a program main function.

The documentation for gofmt states: "Without an explicit path, it processes the standard input." To ensure that TestGoFormatting does not wait on stdin, check that at least one Go file is found. Also, if there is nothing to do then exit.

If possible, make reasonable estimates of slice capacity to minimize allocations.

Read the documentation for filepath.Walk and filepath.WalkFunc.

func Walk

func Walk(root string, walkFn WalkFunc) error

Walk walks the file tree rooted at root, calling walkFn for each file or directory in the tree, including root. All errors that arise visiting files and directories are filtered by walkFn.

type WalkFunc

WalkFunc is the type of the function called for each file or directory visited by Walk. The path argument contains the argument to Walk as a prefix; that is, if Walk is called with "dir", which is a directory containing the file "a", the walk function will be called with argument "dir/a". The info argument is the os.FileInfo for the named path.

If there was a problem walking to the file or directory named by path, the incoming error will describe the problem and the function can decide how to handle that error (and Walk will not descend into that directory). In the case of an error, the info argument will be nil. If an error is returned, processing stops. The sole exception is when the function returns the special value SkipDir. If the function returns SkipDir when invoked on a directory, Walk skips the directory's contents entirely. If the function returns SkipDir when invoked on a non-directory file, Walk skips the remaining files in the containing directory.

type WalkFunc func(path string, info os.FileInfo, err error) error

Write code for the goFiles stub.

func goFiles(root string) ([]string, error) {
    var files []string

    err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
        if err != nil {
            return err
        }
        if info == nil {
            return nil
        }
        if !info.Mode().IsRegular() {
            return nil
        }
        if filepath.Ext(path) != ".go" {
            return nil
        }
        absPath, e := filepath.Abs(path)
        if e != nil {
            return e
        }
        files = append(files, absPath)
        return nil
    })
    if err != nil {
        return nil, err
    }

    return files, nil
}

If an error is passed to the WalkFunc don't ignore it.

Don't panic if the info argument is nil.

The documentation for gofmt states: "Given a file, it operates on that file; given a directory, it operates on all .go files in that directory, recursively."

Only process regular files. If the directory is processed too, the file will be duplicated. For example,

├── fmt.dir.go
│   ├── fmt.file.go

fmt.dir.go and fmt.file.go will be appended to the files list and fmt.file.go will be formatted twice, once as a member of directory fmt.dir.go and once as file fmt.file.go.

Read the documentation for the Go testing package.

Write a Go testing package function.

// TestGoFormatting checks that Go source files have been formatted.
func TestGoFormatting(t *testing.T) {
    // TODO: Set root to a variable value?
    root := "../.."
    out, err := checkGoFmt(root)
    if err != nil {
        t.Error(err)
    }
    if len(out) > 0 {
        t.Error(out)
    }
}

The Go testing package prints the test function name, TestGoFormatting, on failure, There is no need for redundant logging of a test title. TestGoFormatting is more meaningful than TestFormatting. The testing package T.Error method merges the T.Log and T.Fail methods. Eliminate redundant argument error.Error() methods.

Obtaining a value for root requires more thought.

We can also write a program main function.

func main() {
    exit := 0
    // TODO: Set root to a variable value?
    root := "../.."
    out, err := checkGoFmt(root)
    if err != nil {
        exit = 2
        fmt.Fprintln(os.Stderr, err)
    }
    if len(out) > 0 {
        exit = 2
        fmt.Fprintln(os.Stderr, out)
    }
    os.Exit(exit)
}

And so on.

In Go, don't ignore errors.


Here is the complete code for a first draft of TestGoFormatting, a testing function:

package pack

import (
    "os"
    "os/exec"
    "path/filepath"
    "testing"
)

func goFiles(root string) ([]string, error) {
    var files []string

    err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
        if err != nil {
            return err
        }
        if info == nil {
            return nil
        }
        if !info.Mode().IsRegular() {
            return nil
        }
        if filepath.Ext(path) != ".go" {
            return nil
        }
        absPath, e := filepath.Abs(path)
        if e != nil {
            return e
        }
        files = append(files, absPath)
        return nil
    })
    if err != nil {
        return nil, err
    }

    return files, nil
}

// checkGoFmt checks that Go source files have been formatted with go fmt or gofmt.
// checkGoFmt returns a newline separated list of files
// whose formatting differs from gofmt's and any error.
func checkGoFmt(root string) (string, error) {
    // Command gofmt
    // https://golang.org/cmd/gofmt/
    // $ gofmt -help
    // usage: gofmt [flags] [path ...]
    //  -l  list files whose formatting differs from gofmt's
    // $

    files, err := goFiles(root)
    if err != nil {
        return "", err
    }
    if len(files) == 0 {
        return "", nil
    }

    args := make([]string, 0, 1+len(files))
    args = append(args, "-l")
    args = append(args, files...)
    out, err := exec.Command("gofmt", args...).Output()
    return string(out), err
}

// TestGoFormatting checks that Go source files have been formatted.
func TestGoFormatting(t *testing.T) {
    // TODO: Set root to a variable value?
    root := "../.."
    out, err := checkGoFmt(root)
    if err != nil {
        t.Error(err)
    }
    if len(out) > 0 {
        t.Error(out)
    }
}
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.