From 71feecf09c399690d25403256349a7541a183304 Mon Sep 17 00:00:00 2001 From: per1234 Date: Mon, 19 Jul 2021 02:46:36 -0700 Subject: [PATCH 1/3] Fix bug in unit test for result.Record() output An error in the assertion arguments resulted in it providing meaningless results. This also masked the fact that its intended assertion was outdated. --- internal/result/result_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/result/result_test.go b/internal/result/result_test.go index 220e62c2..041558c3 100644 --- a/internal/result/result_test.go +++ b/internal/result/result_test.go @@ -79,7 +79,7 @@ func TestRecord(t *testing.T) { summaryText = results.Record(lintedProject, ruleConfiguration, ruleresult.NotRun, ruleOutput) assert.Equal(t, fmt.Sprintf("Rule %s result: %s\n%s: %s", ruleConfiguration.ID, ruleresult.NotRun, rulelevel.Notice, ruleOutput), summaryText, "Non-fail result should not use message") summaryText = results.Record(lintedProject, ruleConfiguration, ruleresult.Pass, "") - assert.Equal(t, "", "", summaryText, "Non-failure result with no rule function output should result in an empty summary") + assert.Equal(t, fmt.Sprintf("Rule %s result: %s", ruleConfiguration.ID, ruleresult.Pass), summaryText, "Non-failure result with no rule function output should only use preface") flags.Set("verbose", "true") require.Nil(t, configuration.Initialize(flags, projectPaths)) From a8641c39d40a0b3c95e7b9ccea033ceaeaa9e8b4 Mon Sep 17 00:00:00 2001 From: per1234 Date: Mon, 19 Jul 2021 03:00:33 -0700 Subject: [PATCH 2/3] Move rule output control logic to result package Since output always has had the same format regardless of the tool configuration, with the configuration only determining whether or not it will be printed, it made sense to place the printing control logic at the point of printing, always outputting the message from the result package. It has now been determined that there are different formatting requirements for verbose and non-verbose output. This means that it makes more sense to move that logic to the result package and simply return an empty string when no output should be printed. This refactoring in preparation for the formatting changes to the non-verbose output. --- internal/result/result.go | 13 ++++++++----- internal/result/result_test.go | 16 +++++++++++++--- internal/rule/rule.go | 5 +---- 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/internal/result/result.go b/internal/result/result.go index e8923df7..5d033511 100644 --- a/internal/result/result.go +++ b/internal/result/result.go @@ -101,8 +101,6 @@ func (results *Type) Record(lintedProject project.Type, ruleConfiguration ruleco panic(fmt.Errorf("Error while determining rule level: %v", err)) } - summaryText := fmt.Sprintf("Rule %s result: %s", ruleConfiguration.ID, ruleResult) - ruleMessage := "" if ruleResult == ruleresult.Fail { ruleMessage = message(ruleConfiguration.MessageTemplate, ruleOutput) @@ -112,9 +110,14 @@ func (results *Type) Record(lintedProject project.Type, ruleConfiguration ruleco ruleMessage = ruleOutput } - // Add explanation of rule result if present. - if ruleMessage != "" { - summaryText += fmt.Sprintf("\n%s: %s", ruleLevel, ruleMessage) + summaryText := "" + if (ruleResult == ruleresult.Fail) || configuration.Verbose() { + summaryText = fmt.Sprintf("Rule %s result: %s", ruleConfiguration.ID, ruleResult) + // Add explanation of rule result if present. + if ruleMessage != "" { + summaryText += fmt.Sprintf("\n%s: %s", ruleLevel, ruleMessage) + } + summaryText += "\n" } reportExists, projectReportIndex := results.getProjectReportIndex(lintedProject.Path) diff --git a/internal/result/result_test.go b/internal/result/result_test.go index 041558c3..23f21f0c 100644 --- a/internal/result/result_test.go +++ b/internal/result/result_test.go @@ -74,12 +74,22 @@ func TestRecord(t *testing.T) { results.Initialize() ruleConfiguration := ruleconfiguration.Configurations()[0] ruleOutput := "foo" + flags.Set("verbose", "true") + require.Nil(t, configuration.Initialize(flags, projectPaths)) summaryText := results.Record(lintedProject, ruleConfiguration, ruleresult.Fail, ruleOutput) - assert.Equal(t, fmt.Sprintf("Rule %s result: %s\n%s: %s", ruleConfiguration.ID, ruleresult.Fail, rulelevel.Error, message(ruleConfiguration.MessageTemplate, ruleOutput)), summaryText) + assert.Equal(t, fmt.Sprintf("Rule %s result: %s\n%s: %s\n", ruleConfiguration.ID, ruleresult.Fail, rulelevel.Error, message(ruleConfiguration.MessageTemplate, ruleOutput)), summaryText) + summaryText = results.Record(lintedProject, ruleConfiguration, ruleresult.NotRun, ruleOutput) + assert.Equal(t, fmt.Sprintf("Rule %s result: %s\n%s: %s\n", ruleConfiguration.ID, ruleresult.NotRun, rulelevel.Notice, ruleOutput), summaryText, "Non-fail result should not use message") + summaryText = results.Record(lintedProject, ruleConfiguration, ruleresult.Pass, "") + assert.Equal(t, fmt.Sprintf("Rule %s result: %s\n", ruleConfiguration.ID, ruleresult.Pass), summaryText, "Non-failure result with no rule function output should only use preface") + flags.Set("verbose", "false") + require.Nil(t, configuration.Initialize(flags, projectPaths)) + summaryText = results.Record(lintedProject, ruleConfiguration, ruleresult.Fail, ruleOutput) + assert.Equal(t, fmt.Sprintf("Rule %s result: %s\n%s: %s\n", ruleConfiguration.ID, ruleresult.Fail, rulelevel.Error, message(ruleConfiguration.MessageTemplate, ruleOutput)), summaryText) summaryText = results.Record(lintedProject, ruleConfiguration, ruleresult.NotRun, ruleOutput) - assert.Equal(t, fmt.Sprintf("Rule %s result: %s\n%s: %s", ruleConfiguration.ID, ruleresult.NotRun, rulelevel.Notice, ruleOutput), summaryText, "Non-fail result should not use message") + assert.Equal(t, "", summaryText, "Non-fail result should not result in output in non-verbose mode") summaryText = results.Record(lintedProject, ruleConfiguration, ruleresult.Pass, "") - assert.Equal(t, fmt.Sprintf("Rule %s result: %s", ruleConfiguration.ID, ruleresult.Pass), summaryText, "Non-failure result with no rule function output should only use preface") + assert.Equal(t, "", summaryText, "Non-fail result should not result in output in non-verbose mode") flags.Set("verbose", "true") require.Nil(t, configuration.Initialize(flags, projectPaths)) diff --git a/internal/rule/rule.go b/internal/rule/rule.go index f3f677d8..2bd6271a 100644 --- a/internal/rule/rule.go +++ b/internal/rule/rule.go @@ -26,7 +26,6 @@ import ( "github.com/arduino/arduino-lint/internal/result" "github.com/arduino/arduino-lint/internal/result/feedback" "github.com/arduino/arduino-lint/internal/rule/ruleconfiguration" - "github.com/arduino/arduino-lint/internal/rule/ruleresult" "github.com/sirupsen/logrus" ) @@ -52,9 +51,7 @@ func Runner(project project.Type) { ruleResult, ruleOutput := ruleConfiguration.RuleFunction() reportText := result.Results.Record(project, ruleConfiguration, ruleResult, ruleOutput) - if (ruleResult == ruleresult.Fail) || configuration.Verbose() { - feedback.Println(reportText) - } + feedback.Print(reportText) } } From 64551f8289d07df3da71af52aff86615fe4f970d Mon Sep 17 00:00:00 2001 From: per1234 Date: Mon, 19 Jul 2021 03:37:50 -0700 Subject: [PATCH 3/3] Simplify non-verbose rule output Previously, the output resulting from rule violations was always prefaced with the rule ID and result: Rule SD001 result: fail This is necessary in verbose output mode because in that mode the result of every rule applied to the project is printed. This is the only way to identify the rule and result when a rule passes. The situation is different non-verbose mode, where output is only printed when a rule is violated. This means that: - The rule is identified by the rule message - The rule result is implicit So this preface doesn't really serve any purpose in non-verbose mode. It is somewhat cryptic, so may make the output less approachable to the users. Because the rule ID is still sometimes of value as a succinct and unequivocal way to refer to a specific rule, it is printed in a less prominent location at the end of the rule message. --- internal/result/result.go | 6 +++++- internal/result/result_test.go | 2 +- test/test_all.py | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/result/result.go b/internal/result/result.go index 5d033511..2a3b87c7 100644 --- a/internal/result/result.go +++ b/internal/result/result.go @@ -111,13 +111,17 @@ func (results *Type) Record(lintedProject project.Type, ruleConfiguration ruleco } summaryText := "" - if (ruleResult == ruleresult.Fail) || configuration.Verbose() { + if configuration.Verbose() { summaryText = fmt.Sprintf("Rule %s result: %s", ruleConfiguration.ID, ruleResult) // Add explanation of rule result if present. if ruleMessage != "" { summaryText += fmt.Sprintf("\n%s: %s", ruleLevel, ruleMessage) } summaryText += "\n" + } else { + if ruleResult == ruleresult.Fail { + summaryText = fmt.Sprintf("%s: %s (Rule %s)\n", ruleLevel, ruleMessage, ruleConfiguration.ID) + } } reportExists, projectReportIndex := results.getProjectReportIndex(lintedProject.Path) diff --git a/internal/result/result_test.go b/internal/result/result_test.go index 23f21f0c..e79c4625 100644 --- a/internal/result/result_test.go +++ b/internal/result/result_test.go @@ -85,7 +85,7 @@ func TestRecord(t *testing.T) { flags.Set("verbose", "false") require.Nil(t, configuration.Initialize(flags, projectPaths)) summaryText = results.Record(lintedProject, ruleConfiguration, ruleresult.Fail, ruleOutput) - assert.Equal(t, fmt.Sprintf("Rule %s result: %s\n%s: %s\n", ruleConfiguration.ID, ruleresult.Fail, rulelevel.Error, message(ruleConfiguration.MessageTemplate, ruleOutput)), summaryText) + assert.Equal(t, fmt.Sprintf("%s: %s (Rule %s)\n", rulelevel.Error, message(ruleConfiguration.MessageTemplate, ruleOutput), ruleConfiguration.ID), summaryText) summaryText = results.Record(lintedProject, ruleConfiguration, ruleresult.NotRun, ruleOutput) assert.Equal(t, "", summaryText, "Non-fail result should not result in output in non-verbose mode") summaryText = results.Record(lintedProject, ruleConfiguration, ruleresult.Pass, "") diff --git a/test/test_all.py b/test/test_all.py index 863a0be8..038572a1 100644 --- a/test/test_all.py +++ b/test/test_all.py @@ -181,7 +181,7 @@ def test_verbose(run_command): result = run_command(cmd=["--format", "text", project_path]) assert result.ok assert "result: pass" not in result.stdout - assert "result: fail" in result.stdout + assert "WARNING:" in result.stdout result = run_command(cmd=["--format", "text", "--verbose", project_path]) assert result.ok