From e0099885fe37575b41b3112c458bc86fb37b5280 Mon Sep 17 00:00:00 2001 From: Jeevanandam M Date: Thu, 9 Jun 2016 10:33:02 -0700 Subject: [PATCH] revel/revel#1057 code improvements docs, errcheck, cyclo, etc --- harness/app.go | 4 + harness/build.go | 4 + harness/harness.go | 9 +- harness/reflect.go | 4 + harness/reflect_test.go | 4 + revel/build.go | 23 ++-- revel/clean.go | 4 + revel/new.go | 6 +- revel/package.go | 10 +- revel/rev.go | 8 ++ revel/run.go | 4 + revel/test.go | 241 ++++++++++++++++++++++------------------ revel/util.go | 28 +++-- revel/version.go | 4 + 14 files changed, 227 insertions(+), 126 deletions(-) diff --git a/harness/app.go b/harness/app.go index 63e0060f..c1b142ca 100644 --- a/harness/app.go +++ b/harness/app.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package harness import ( diff --git a/harness/build.go b/harness/build.go index 4b4d5a19..4e9c6728 100755 --- a/harness/build.go +++ b/harness/build.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package harness import ( diff --git a/harness/harness.go b/harness/harness.go index 4d467133..ccb877e6 100644 --- a/harness/harness.go +++ b/harness/harness.go @@ -1,13 +1,16 @@ -// The Harness for a Revel program. +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + +// Package harness for a Revel Framework. // -// It has a couple responsibilities: +// It has a following responsibilities: // 1. Parse the user program, generating a main.go file that registers // controller classes and starts the user's server. // 2. Build and run the user program. Show compile errors. // 3. Monitor the user source and re-build / restart the program when necessary. // // Source files are generated in the app/tmp directory. - package harness import ( diff --git a/harness/reflect.go b/harness/reflect.go index 853f76c4..ded4b7d4 100644 --- a/harness/reflect.go +++ b/harness/reflect.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package harness // This file handles the app code introspection. diff --git a/harness/reflect_test.go b/harness/reflect_test.go index 9815ca29..9db9b23f 100644 --- a/harness/reflect_test.go +++ b/harness/reflect_test.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package harness import ( diff --git a/revel/build.go b/revel/build.go index 25d2524c..58ee4e33 100644 --- a/revel/build.go +++ b/revel/build.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package main import ( @@ -40,7 +44,7 @@ func buildApp(args []string) { return } - appImportPath, destPath, mode := args[0], args[1], "dev" + appImportPath, destPath, mode := args[0], args[1], DefaultRunMode if len(args) >= 3 { mode = args[2] } @@ -55,8 +59,13 @@ func buildApp(args []string) { errorf("Abort: %s exists and does not look like a build directory.", destPath) } - os.RemoveAll(destPath) - os.MkdirAll(destPath, 0777) + if err := os.RemoveAll(destPath); err != nil && !os.IsNotExist(err) { + revel.ERROR.Fatalln(err) + } + + if err := os.MkdirAll(destPath, 0777); err != nil { + revel.ERROR.Fatalln(err) + } app, reverr := harness.Build() panicOnError(reverr, "Failed to build") @@ -73,9 +82,9 @@ func buildApp(args []string) { tmpRevelPath := filepath.Join(srcPath, filepath.FromSlash(revel.RevelImportPath)) mustCopyFile(destBinaryPath, app.BinaryPath) mustChmod(destBinaryPath, 0755) - mustCopyDir(filepath.Join(tmpRevelPath, "conf"), filepath.Join(revel.RevelPath, "conf"), nil) - mustCopyDir(filepath.Join(tmpRevelPath, "templates"), filepath.Join(revel.RevelPath, "templates"), nil) - mustCopyDir(filepath.Join(srcPath, filepath.FromSlash(appImportPath)), revel.BasePath, nil) + _ = mustCopyDir(filepath.Join(tmpRevelPath, "conf"), filepath.Join(revel.RevelPath, "conf"), nil) + _ = mustCopyDir(filepath.Join(tmpRevelPath, "templates"), filepath.Join(revel.RevelPath, "templates"), nil) + _ = mustCopyDir(filepath.Join(srcPath, filepath.FromSlash(appImportPath)), revel.BasePath, nil) // Find all the modules used and copy them over. config := revel.Config.Raw() @@ -98,7 +107,7 @@ func buildApp(args []string) { } } for importPath, fsPath := range modulePaths { - mustCopyDir(filepath.Join(srcPath, importPath), fsPath, nil) + _ = mustCopyDir(filepath.Join(srcPath, importPath), fsPath, nil) } tmplData, runShPath := map[string]interface{}{ diff --git a/revel/clean.go b/revel/clean.go index 254b5497..0a0126a4 100644 --- a/revel/clean.go +++ b/revel/clean.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package main import ( diff --git a/revel/new.go b/revel/new.go index 1ada576e..306672d5 100644 --- a/revel/new.go +++ b/revel/new.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package main import ( @@ -199,7 +203,7 @@ func copyNewAppFiles() { err = os.MkdirAll(appPath, 0777) panicOnError(err, "Failed to create directory "+appPath) - mustCopyDir(appPath, skeletonPath, map[string]interface{}{ + _ = mustCopyDir(appPath, skeletonPath, map[string]interface{}{ // app.conf "AppName": appName, "BasePath": basePath, diff --git a/revel/package.go b/revel/package.go index 41ea785c..0eb4d401 100644 --- a/revel/package.go +++ b/revel/package.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package main import ( @@ -38,7 +42,7 @@ func packageApp(args []string) { } // Determine the run mode. - mode := "dev" + mode := DefaultRunMode if len(args) >= 2 { mode = args[1] } @@ -48,7 +52,9 @@ func packageApp(args []string) { // Remove the archive if it already exists. destFile := filepath.Base(revel.BasePath) + ".tar.gz" - os.Remove(destFile) + if err := os.Remove(destFile); err != nil && !os.IsNotExist(err) { + revel.ERROR.Fatal(err) + } // Collect stuff in a temp directory. tmpDir, err := ioutil.TempDir("", filepath.Base(revel.BasePath)) diff --git a/revel/rev.go b/revel/rev.go index 6c234b80..26918266 100644 --- a/revel/rev.go +++ b/revel/rev.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + // The command line tool for running Revel apps. package main @@ -15,12 +19,16 @@ import ( "github.com/agtorre/gocolorize" ) +// DefaultRunMode for revel's application +const DefaultRunMode = "dev" + // Command structure cribbed from the genius organization of the "go" command. type Command struct { Run func(args []string) UsageLine, Short, Long string } +// Name returns command name from usage line func (cmd *Command) Name() string { name := cmd.UsageLine i := strings.Index(name, " ") diff --git a/revel/run.go b/revel/run.go index 4d4572f4..fa231548 100644 --- a/revel/run.go +++ b/revel/run.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package main import ( diff --git a/revel/test.go b/revel/test.go index ac01bc28..10d5bac8 100644 --- a/revel/test.go +++ b/revel/test.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package main import ( @@ -52,7 +56,7 @@ func testApp(args []string) { errorf("No import path given.\nRun 'revel help test' for usage.\n") } - mode := "dev" + mode := DefaultRunMode if len(args) >= 2 { mode = args[1] } @@ -61,22 +65,7 @@ func testApp(args []string) { revel.Init(mode, args[0], "") // Ensure that the testrunner is loaded in this mode. - testRunnerFound := false - for _, module := range revel.Modules { - if module.ImportPath == revel.Config.StringDefault("module.testrunner", "github.com/revel/modules/testrunner") { - testRunnerFound = true - break - } - } - if !testRunnerFound { - errorf(`Error: The testrunner module is not running. - -You can add it to a run mode configuration with the following line: - - module.testrunner = github.com/revel/modules/testrunner - -`) - } + checkTestRunner() // Create a directory to hold the test result files. resultPath := filepath.Join(revel.BasePath, "test-results") @@ -108,16 +97,121 @@ You can add it to a run mode configuration with the following line: defer cmd.Kill() revel.INFO.Printf("Testing %s (%s) in %s mode\n", revel.AppName, revel.ImportPath, mode) - // Get a list of tests. - // Since this is the first request to the server, retry/sleep a couple times - // in case it hasn't finished starting up yet. + // Get a list of tests + var baseURL = fmt.Sprintf("http://127.0.0.1:%d", revel.HTTPPort) + testSuites, _ := getTestsList(baseURL) + + // If a specific TestSuite[.Method] is specified, only run that suite/test + if len(args) == 3 { + testSuites = filterTestSuites(testSuites, args[2]) + } + testSuiteCount := len(*testSuites) + fmt.Printf("\n%d test suite%s to run.\n", testSuiteCount, pluralize(testSuiteCount, "", "s")) + fmt.Println() + + // Run each suite. + failedResults, overallSuccess := runTestSuites(baseURL, resultPath, testSuites) + + fmt.Println() + if overallSuccess { + writeResultFile(resultPath, "result.passed", "passed") + fmt.Println("All Tests Passed.") + } else { + for _, failedResult := range *failedResults { + fmt.Printf("Failures:\n") + for _, result := range failedResult.Results { + if !result.Passed { + fmt.Printf("%s.%s\n", failedResult.Name, result.Name) + fmt.Printf("%s\n\n", result.ErrorSummary) + } + } + } + writeResultFile(resultPath, "result.failed", "failed") + errorf("Some tests failed. See file://%s for results.", resultPath) + } +} + +func writeResultFile(resultPath, name, content string) { + if err := ioutil.WriteFile(filepath.Join(resultPath, name), []byte(content), 0666); err != nil { + errorf("Failed to write result file %s: %s", filepath.Join(resultPath, name), err) + } +} + +func pluralize(num int, singular, plural string) string { + if num == 1 { + return singular + } + return plural +} + +// Filters test suites and individual tests to match +// the parsed command line parameter +func filterTestSuites(suites *[]controllers.TestSuiteDesc, suiteArgument string) *[]controllers.TestSuiteDesc { + var suiteName, testName string + argArray := strings.Split(suiteArgument, ".") + suiteName = argArray[0] + if suiteName == "" { + return suites + } + if len(argArray) == 2 { + testName = argArray[1] + } + for _, suite := range *suites { + if suite.Name != suiteName { + continue + } + if testName == "" { + return &[]controllers.TestSuiteDesc{suite} + } + // Only run a particular test in a suite + for _, test := range suite.Tests { + if test.Name != testName { + continue + } + return &[]controllers.TestSuiteDesc{ + { + Name: suite.Name, + Tests: []controllers.TestDesc{test}, + }, + } + } + errorf("Couldn't find test %s in suite %s", testName, suiteName) + } + errorf("Couldn't find test suite %s", suiteName) + return nil +} + +func checkTestRunner() { + testRunnerFound := false + for _, module := range revel.Modules { + if module.ImportPath == revel.Config.StringDefault("module.testrunner", "github.com/revel/modules/testrunner") { + testRunnerFound = true + break + } + } + + if !testRunnerFound { + errorf(`Error: The testrunner module is not running. + +You can add it to a run mode configuration with the following line: + + module.testrunner = github.com/revel/modules/testrunner + +`) + } +} + +// Get a list of tests from server. +// Since this is the first request to the server, retry/sleep a couple times +// in case it hasn't finished starting up yet. +func getTestsList(baseURL string) (*[]controllers.TestSuiteDesc, error) { var ( - testSuites []controllers.TestSuiteDesc + err error resp *http.Response - baseUrl = fmt.Sprintf("http://127.0.0.1:%d", revel.HTTPPort) + testSuites []controllers.TestSuiteDesc ) for i := 0; ; i++ { - if resp, err = http.Get(baseUrl + "/@tests.list"); err == nil { + if resp, err = http.Get(baseURL + "/@tests.list"); err == nil { if resp.StatusCode == http.StatusOK { break } @@ -132,16 +226,16 @@ You can add it to a run mode configuration with the following line: errorf("Failed to request test list: non-200 response") } } - defer resp.Body.Close() - json.NewDecoder(resp.Body).Decode(&testSuites) + defer func() { + _ = resp.Body.Close() + }() - // If a specific TestSuite[.Method] is specified, only run that suite/test - if len(args) == 3 { - testSuites = filterTestSuites(testSuites, args[2]) - } - fmt.Printf("\n%d test suite%s to run.\n", len(testSuites), pluralize(len(testSuites), "", "s")) - fmt.Println() + err = json.NewDecoder(resp.Body).Decode(&testSuites) + + return &testSuites, err +} +func runTestSuites(baseURL, resultPath string, testSuites *[]controllers.TestSuiteDesc) (*[]controllers.TestSuiteResult, bool) { // Load the result template, which we execute for each suite. module, _ := revel.ModuleByName("testrunner") TemplateLoader := revel.NewTemplateLoader([]string{filepath.Join(module.Path, "app", "views")}) @@ -153,12 +247,11 @@ You can add it to a run mode configuration with the following line: errorf("Failed to load suite result template: %s", err) } - // Run each suite. var ( overallSuccess = true failedResults []controllers.TestSuiteResult ) - for _, suite := range testSuites { + for _, suite := range *testSuites { // Print the name of the suite we're running. name := suite.Name if len(name) > 22 { @@ -170,16 +263,18 @@ You can add it to a run mode configuration with the following line: startTime := time.Now() suiteResult := controllers.TestSuiteResult{Name: suite.Name, Passed: true} for _, test := range suite.Tests { - testUrl := baseUrl + "/@tests/" + suite.Name + "/" + test.Name - resp, err := http.Get(testUrl) + testURL := baseURL + "/@tests/" + suite.Name + "/" + test.Name + resp, err := http.Get(testURL) if err != nil { - errorf("Failed to fetch test result at url %s: %s", testUrl, err) + errorf("Failed to fetch test result at url %s: %s", testURL, err) } - defer resp.Body.Close() + defer func() { + _ = resp.Body.Close() + }() var testResult controllers.TestResult - json.NewDecoder(resp.Body).Decode(&testResult) - if !testResult.Passed { + err = json.NewDecoder(resp.Body).Decode(&testResult) + if err == nil && !testResult.Passed { suiteResult.Passed = false } suiteResult.Results = append(suiteResult.Results, testResult) @@ -205,71 +300,5 @@ You can add it to a run mode configuration with the following line: } } - fmt.Println() - if overallSuccess { - writeResultFile(resultPath, "result.passed", "passed") - fmt.Println("All Tests Passed.") - } else { - for _, failedResult := range failedResults { - fmt.Printf("Failures:\n") - for _, result := range failedResult.Results { - if !result.Passed { - fmt.Printf("%s.%s\n", failedResult.Name, result.Name) - fmt.Printf("%s\n\n", result.ErrorSummary) - } - } - } - writeResultFile(resultPath, "result.failed", "failed") - errorf("Some tests failed. See file://%s for results.", resultPath) - } -} - -func writeResultFile(resultPath, name, content string) { - if err := ioutil.WriteFile(filepath.Join(resultPath, name), []byte(content), 0666); err != nil { - errorf("Failed to write result file %s: %s", filepath.Join(resultPath, name), err) - } -} - -func pluralize(num int, singular, plural string) string { - if num == 1 { - return singular - } - return plural -} - -// Filters test suites and individual tests to match -// the parsed command line parameter -func filterTestSuites(suites []controllers.TestSuiteDesc, suiteArgument string) []controllers.TestSuiteDesc { - var suiteName, testName string - argArray := strings.Split(suiteArgument, ".") - suiteName = argArray[0] - if suiteName == "" { - return suites - } - if len(argArray) == 2 { - testName = argArray[1] - } - for _, suite := range suites { - if suite.Name != suiteName { - continue - } - if testName == "" { - return []controllers.TestSuiteDesc{suite} - } - // Only run a particular test in a suite - for _, test := range suite.Tests { - if test.Name != testName { - continue - } - return []controllers.TestSuiteDesc{ - controllers.TestSuiteDesc{ - Name: suite.Name, - Tests: []controllers.TestDesc{test}, - }, - } - } - errorf("Couldn't find test %s in suite %s", testName, suiteName) - } - errorf("Couldn't find test suite %s", suiteName) - return nil + return &failedResults, overallSuccess } diff --git a/revel/util.go b/revel/util.go index b15fd7c8..9535cb67 100644 --- a/revel/util.go +++ b/revel/util.go @@ -1,3 +1,7 @@ +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package main import ( @@ -13,7 +17,7 @@ import ( "github.com/revel/revel" ) -// Use a wrapper to differentiate logged panics from unexpected ones. +// LoggedError is wrapper to differentiate logged panics from unexpected ones. type LoggedError struct{ error } func panicOnError(err error, msg string) { @@ -103,22 +107,30 @@ func mustCopyDir(destDir, srcDir string, data map[string]interface{}) error { func mustTarGzDir(destFilename, srcDir string) string { zipFile, err := os.Create(destFilename) panicOnError(err, "Failed to create archive") - defer zipFile.Close() + defer func() { + _ = zipFile.Close() + }() gzipWriter := gzip.NewWriter(zipFile) - defer gzipWriter.Close() + defer func() { + _ = gzipWriter.Close() + }() tarWriter := tar.NewWriter(gzipWriter) - defer tarWriter.Close() + defer func() { + _ = tarWriter.Close() + }() - revel.Walk(srcDir, func(srcPath string, info os.FileInfo, err error) error { + _ = revel.Walk(srcDir, func(srcPath string, info os.FileInfo, err error) error { if info.IsDir() { return nil } srcFile, err := os.Open(srcPath) panicOnError(err, "Failed to read source file") - defer srcFile.Close() + defer func() { + _ = srcFile.Close() + }() err = tarWriter.WriteHeader(&tar.Header{ Name: strings.TrimLeft(srcPath[len(srcDir):], string(os.PathSeparator)), @@ -149,7 +161,9 @@ func empty(dirname string) bool { if err != nil { errorf("error opening directory: %s", err) } - defer dir.Close() + defer func() { + _ = dir.Close() + }() results, _ := dir.Readdir(1) return len(results) == 0 } diff --git a/revel/version.go b/revel/version.go index 7343b1c6..c6ee21aa 100644 --- a/revel/version.go +++ b/revel/version.go @@ -2,6 +2,10 @@ // Revel Framework source code and usage is governed by a MIT style // license that can be found in the LICENSE file. +// Copyright (c) 2012-2016 The Revel Framework Authors, All rights reserved. +// Revel Framework source code and usage is governed by a MIT style +// license that can be found in the LICENSE file. + package main import (