Merge pull request #89 from arduino/per1234/improve-check-output

Improve handling of check output

Author: per1234

Diff for: check/checkconfigurations/checkconfigurations_test.go

@@ -32,7 +32,7 @@ func TestConfigurationResolution(t *testing.T) {
 			enabled, err := check.IsEnabled(checkConfiguration, map[checkmode.Type]bool{checkMode: true})
 			assert.Nil(t, err, fmt.Sprintf("Enable configuration of check %s doesn't resolve for check mode %s", checkConfiguration.ID, checkMode))
 			if err == nil && enabled {
-				_, err := checklevel.CheckLevelForCheckModes(checkConfiguration, map[checkmode.Type]bool{checkMode: true})
+				_, err := checklevel.FailCheckLevel(checkConfiguration, map[checkmode.Type]bool{checkMode: true})
 				assert.Nil(t, err, fmt.Sprintf("Level configuration of check %s doesn't resolve for check mode %s", checkConfiguration.ID, checkMode))
 			}
 		}

Diff for: check/checklevel/checklevel.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 
 	"github.com/arduino/arduino-check/check/checkconfigurations"
+	"github.com/arduino/arduino-check/check/checkresult"
 	"github.com/arduino/arduino-check/configuration"
 	"github.com/arduino/arduino-check/configuration/checkmode"
 )
@@ -36,13 +37,17 @@ const (
 	Notice              // notice
 )
 
-// CheckLevel determines the check level assigned to failure of the given check under the current tool configuration.
-func CheckLevel(checkConfiguration checkconfigurations.Type) (Type, error) {
+// CheckLevel determines the check level assigned to the given result of the given check under the current tool configuration.
+func CheckLevel(checkConfiguration checkconfigurations.Type, checkResult checkresult.Type) (Type, error) {
+	if checkResult != checkresult.Fail {
+		return Notice, nil // Level provided by FailCheckLevel() is only relevant for failure result.
+	}
 	configurationCheckModes := configuration.CheckModes(checkConfiguration.ProjectType)
-	return CheckLevelForCheckModes(checkConfiguration, configurationCheckModes)
+	return FailCheckLevel(checkConfiguration, configurationCheckModes)
 }
 
-func CheckLevelForCheckModes(checkConfiguration checkconfigurations.Type, configurationCheckModes map[checkmode.Type]bool) (Type, error) {
+// FailCheckLevel determines the level of a failed check for the given check modes.
+func FailCheckLevel(checkConfiguration checkconfigurations.Type, configurationCheckModes map[checkmode.Type]bool) (Type, error) {
 	for _, errorMode := range checkConfiguration.ErrorModes {
 		if configurationCheckModes[errorMode] {
 			return Error, nil

Diff for: check/checklevel/checklevel_test.go

@@ -19,6 +19,7 @@ import (
 	"testing"
 
 	"github.com/arduino/arduino-check/check/checkconfigurations"
+	"github.com/arduino/arduino-check/check/checkresult"
 	"github.com/arduino/arduino-check/configuration"
 	"github.com/arduino/arduino-check/configuration/checkmode"
 	"github.com/arduino/arduino-check/util/test"
@@ -31,19 +32,21 @@ func TestCheckLevel(t *testing.T) {
 		infoModes             []checkmode.Type
 		warningModes          []checkmode.Type
 		errorModes            []checkmode.Type
+		checkResult           checkresult.Type
 		libraryManagerSetting string
 		permissiveSetting     string
 		expectedLevel         Type
 		errorAssertion        assert.ErrorAssertionFunc
 	}{
-		{"Error", []checkmode.Type{}, []checkmode.Type{}, []checkmode.Type{checkmode.LibraryManagerSubmission}, "submit", "false", Error, assert.NoError},
-		{"Warning", []checkmode.Type{}, []checkmode.Type{checkmode.LibraryManagerSubmission}, []checkmode.Type{}, "submit", "false", Warning, assert.NoError},
-		{"Info", []checkmode.Type{checkmode.LibraryManagerSubmission}, []checkmode.Type{}, []checkmode.Type{}, "submit", "false", Info, assert.NoError},
-		{"Default to Error", []checkmode.Type{}, []checkmode.Type{}, []checkmode.Type{checkmode.Default}, "submit", "false", Error, assert.NoError},
-		{"Default to Warning", []checkmode.Type{}, []checkmode.Type{checkmode.Default}, []checkmode.Type{}, "submit", "false", Warning, assert.NoError},
-		{"Default to Info", []checkmode.Type{checkmode.Default}, []checkmode.Type{}, []checkmode.Type{}, "submit", "false", Info, assert.NoError},
-		{"Default override", []checkmode.Type{checkmode.Default}, []checkmode.Type{}, []checkmode.Type{checkmode.LibraryManagerSubmission}, "submit", "false", Error, assert.NoError},
-		{"Unable to resolve", []checkmode.Type{}, []checkmode.Type{}, []checkmode.Type{}, "submit", "false", Info, assert.Error},
+		{"Non-fail", []checkmode.Type{}, []checkmode.Type{}, []checkmode.Type{checkmode.LibraryManagerSubmission}, checkresult.Skip, "submit", "false", Notice, assert.NoError},
+		{"Error", []checkmode.Type{}, []checkmode.Type{}, []checkmode.Type{checkmode.LibraryManagerSubmission}, checkresult.Fail, "submit", "false", Error, assert.NoError},
+		{"Warning", []checkmode.Type{}, []checkmode.Type{checkmode.LibraryManagerSubmission}, []checkmode.Type{}, checkresult.Fail, "submit", "false", Warning, assert.NoError},
+		{"Info", []checkmode.Type{checkmode.LibraryManagerSubmission}, []checkmode.Type{}, []checkmode.Type{}, checkresult.Fail, "submit", "false", Info, assert.NoError},
+		{"Default to Error", []checkmode.Type{}, []checkmode.Type{}, []checkmode.Type{checkmode.Default}, checkresult.Fail, "submit", "false", Error, assert.NoError},
+		{"Default to Warning", []checkmode.Type{}, []checkmode.Type{checkmode.Default}, []checkmode.Type{}, checkresult.Fail, "submit", "false", Warning, assert.NoError},
+		{"Default to Info", []checkmode.Type{checkmode.Default}, []checkmode.Type{}, []checkmode.Type{}, checkresult.Fail, "submit", "false", Info, assert.NoError},
+		{"Default override", []checkmode.Type{checkmode.Default}, []checkmode.Type{}, []checkmode.Type{checkmode.LibraryManagerSubmission}, checkresult.Fail, "submit", "false", Error, assert.NoError},
+		{"Unable to resolve", []checkmode.Type{}, []checkmode.Type{}, []checkmode.Type{}, checkresult.Fail, "submit", "false", Info, assert.Error},
 	}
 
 	flags := test.ConfigurationFlags()
@@ -60,10 +63,10 @@ func TestCheckLevel(t *testing.T) {
 			ErrorModes:   testTable.errorModes,
 		}
 
-		level, err := CheckLevel(checkConfiguration)
-		testTable.errorAssertion(t, err)
+		level, err := CheckLevel(checkConfiguration, testTable.checkResult)
+		testTable.errorAssertion(t, err, testTable.testName)
 		if err == nil {
-			assert.Equal(t, testTable.expectedLevel, level)
+			assert.Equal(t, testTable.expectedLevel, level, testTable.testName)
 		}
 	}
 }

Diff for: result/result.go

@@ -92,20 +92,25 @@ func (results *Type) Initialize() {
 
 // Record records the result of a check and returns a text summary for it.
 func (results *Type) Record(checkedProject project.Type, checkConfiguration checkconfigurations.Type, checkResult checkresult.Type, checkOutput string) string {
-	checkMessage := message(checkConfiguration.MessageTemplate, checkOutput)
-
-	checkLevel, err := checklevel.CheckLevel(checkConfiguration)
+	checkLevel, err := checklevel.CheckLevel(checkConfiguration, checkResult)
 	if err != nil {
 		feedback.Errorf("Error while determining check level: %v", err)
 		os.Exit(1)
 	}
 
 	summaryText := fmt.Sprintf("%s\n", checkResult)
 
-	if checkResult == checkresult.NotRun {
-		// TODO: make the check functions output an explanation for why they didn't run
-		summaryText += fmt.Sprintf("%s: %s\n", checklevel.Notice, checkOutput)
-	} else if checkResult != checkresult.Pass {
+	checkMessage := ""
+	if checkLevel == checklevel.Error {
+		checkMessage = message(checkConfiguration.MessageTemplate, checkOutput)
+	} else {
+		// Checks may provide an explanation for their non-fail result.
+		// The message template should not be used in this case, since it is written for a failure result.
+		checkMessage = checkOutput
+	}
+
+	// Add explanation of check result if present.
+	if checkMessage != "" {
 		summaryText += fmt.Sprintf("%s: %s\n", checkLevel, checkMessage)
 	}
 

Diff for: result/result_test.go

@@ -61,12 +61,12 @@ func TestRecord(t *testing.T) {
 	var results Type
 	checkConfiguration := checkconfigurations.Configurations()[0]
 	checkOutput := "foo"
-	summaryText := results.Record(checkedProject, checkConfiguration, checkresult.Pass, checkOutput)
-	assert.Equal(t, fmt.Sprintf("%s\n", checkresult.Pass), summaryText)
-	summaryText = results.Record(checkedProject, checkConfiguration, checkresult.NotRun, checkOutput)
-	assert.Equal(t, fmt.Sprintf("%s\n%s: %s\n", checkresult.NotRun, checklevel.Notice, checkOutput), summaryText)
-	summaryText = results.Record(checkedProject, checkConfiguration, checkresult.Fail, checkOutput)
+	summaryText := results.Record(checkedProject, checkConfiguration, checkresult.Fail, checkOutput)
 	assert.Equal(t, fmt.Sprintf("%s\n%s: %s\n", checkresult.Fail, checklevel.Error, message(checkConfiguration.MessageTemplate, checkOutput)), summaryText)
+	summaryText = results.Record(checkedProject, checkConfiguration, checkresult.NotRun, checkOutput)
+	assert.Equal(t, fmt.Sprintf("%s\n%s: %s\n", checkresult.NotRun, checklevel.Notice, checkOutput), summaryText, "Non-fail result should not use message")
+	summaryText = results.Record(checkedProject, checkConfiguration, checkresult.Pass, "")
+	assert.Equal(t, "", "", summaryText, "Non-failure result with no check function output should result in an empty summary")
 
 	checkResult := checkresult.Pass
 	results = Type{}
@@ -88,9 +88,9 @@ func TestRecord(t *testing.T) {
 	assert.Equal(t, checkConfiguration.Brief, checkReport.Brief)
 	assert.Equal(t, checkConfiguration.Description, checkReport.Description)
 	assert.Equal(t, checkResult.String(), checkReport.Result)
-	checkLevel, _ := checklevel.CheckLevel(checkConfiguration)
+	checkLevel, _ := checklevel.CheckLevel(checkConfiguration, checkResult)
 	assert.Equal(t, checkLevel.String(), checkReport.Level)
-	assert.Equal(t, message(checkConfiguration.MessageTemplate, checkOutput), checkReport.Message)
+	assert.Equal(t, checkOutput, checkReport.Message)
 
 	assert.Len(t, results.Projects, 1)
 	previousProjectPath := checkedProject.Path