Skip to content

Fix bugs in check functions #96

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Dec 11, 2020
81 changes: 52 additions & 29 deletions check/checkfunctions/library.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
"github.com/arduino/arduino-check/project/sketch"
"github.com/arduino/arduino-cli/arduino/libraries"
"github.com/arduino/arduino-cli/arduino/utils"
"github.com/arduino/go-properties-orderedmap"
"github.com/go-git/go-git/v5"
"github.com/go-git/go-git/v5/plumbing"
"github.com/go-git/go-git/v5/plumbing/object"
Expand Down Expand Up @@ -111,8 +110,9 @@ func RedundantLibraryProperties() (result checkresult.Type, output string) {

// LibraryPropertiesNameFieldMissing checks for missing library.properties "name" field.
func LibraryPropertiesNameFieldMissing() (result checkresult.Type, output string) {
if checkdata.LibraryPropertiesLoadError() != nil {
return checkresult.NotRun, "Couldn't load library.properties"
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
if !shouldRun {
return checkresult.NotRun, reason
}

if schema.RequiredPropertyMissing("name", checkdata.LibraryPropertiesSchemaValidationResult()[compliancelevel.Specification], configuration.SchemasPath()) {
Expand Down Expand Up @@ -185,7 +185,7 @@ func LibraryPropertiesNameFieldDisallowedCharacters() (result checkresult.Type,
return checkresult.NotRun, "Field not present"
}

if schema.PropertyPatternMismatch("name", checkdata.LibraryPropertiesSchemaValidationResult()[compliancelevel.Specification], configuration.SchemasPath()) {
if schema.ValidationErrorMatch("^#/name$", "/patternObjects/allowedCharacters", "", "", checkdata.LibraryPropertiesSchemaValidationResult()[compliancelevel.Specification], configuration.SchemasPath()) {
return checkresult.Fail, name
}

Expand Down Expand Up @@ -340,8 +340,9 @@ func LibraryPropertiesNameFieldHeaderMismatch() (result checkresult.Type, output

// LibraryPropertiesVersionFieldMissing checks for missing library.properties "version" field.
func LibraryPropertiesVersionFieldMissing() (result checkresult.Type, output string) {
if checkdata.LibraryPropertiesLoadError() != nil {
return checkresult.NotRun, "Couldn't load library.properties"
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
if !shouldRun {
return checkresult.NotRun, reason
}

if schema.RequiredPropertyMissing("version", checkdata.LibraryPropertiesSchemaValidationResult()[compliancelevel.Specification], configuration.SchemasPath()) {
Expand Down Expand Up @@ -481,8 +482,9 @@ func LibraryPropertiesVersionFieldBehindTag() (result checkresult.Type, output s

// LibraryPropertiesAuthorFieldMissing checks for missing library.properties "author" field.
func LibraryPropertiesAuthorFieldMissing() (result checkresult.Type, output string) {
if checkdata.LibraryPropertiesLoadError() != nil {
return checkresult.NotRun, "Couldn't load library.properties"
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
if !shouldRun {
return checkresult.NotRun, reason
}

if schema.RequiredPropertyMissing("author", checkdata.LibraryPropertiesSchemaValidationResult()[compliancelevel.Specification], configuration.SchemasPath()) {
Expand Down Expand Up @@ -510,8 +512,9 @@ func LibraryPropertiesAuthorFieldLTMinLength() (result checkresult.Type, output

// LibraryPropertiesMaintainerFieldMissing checks for missing library.properties "maintainer" field.
func LibraryPropertiesMaintainerFieldMissing() (result checkresult.Type, output string) {
if checkdata.LibraryPropertiesLoadError() != nil {
return checkresult.NotRun, "Couldn't load library.properties"
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
if !shouldRun {
return checkresult.NotRun, reason
}

if schema.RequiredPropertyMissing("maintainer", checkdata.LibraryPropertiesSchemaValidationResult()[compliancelevel.Specification], configuration.SchemasPath()) {
Expand Down Expand Up @@ -613,8 +616,9 @@ func LibraryPropertiesEmailFieldStartsWithArduino() (result checkresult.Type, ou

// LibraryPropertiesSentenceFieldMissing checks for missing library.properties "sentence" field.
func LibraryPropertiesSentenceFieldMissing() (result checkresult.Type, output string) {
if checkdata.LibraryPropertiesLoadError() != nil {
return checkresult.NotRun, "Couldn't load library.properties"
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
if !shouldRun {
return checkresult.NotRun, reason
}

if schema.RequiredPropertyMissing("sentence", checkdata.LibraryPropertiesSchemaValidationResult()[compliancelevel.Specification], configuration.SchemasPath()) {
Expand Down Expand Up @@ -647,8 +651,9 @@ func LibraryPropertiesSentenceFieldSpellCheck() (result checkresult.Type, output

// LibraryPropertiesParagraphFieldMissing checks for missing library.properties "paragraph" field.
func LibraryPropertiesParagraphFieldMissing() (result checkresult.Type, output string) {
if checkdata.LibraryPropertiesLoadError() != nil {
return checkresult.NotRun, "Couldn't load library.properties"
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
if !shouldRun {
return checkresult.NotRun, reason
}

if schema.RequiredPropertyMissing("paragraph", checkdata.LibraryPropertiesSchemaValidationResult()[compliancelevel.Specification], configuration.SchemasPath()) {
Expand Down Expand Up @@ -683,8 +688,9 @@ func LibraryPropertiesParagraphFieldRepeatsSentence() (result checkresult.Type,

// LibraryPropertiesCategoryFieldMissing checks for missing library.properties "category" field.
func LibraryPropertiesCategoryFieldMissing() (result checkresult.Type, output string) {
if checkdata.LibraryPropertiesLoadError() != nil {
return checkresult.NotRun, "Couldn't load library.properties"
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
if !shouldRun {
return checkresult.NotRun, reason
}

if schema.RequiredPropertyMissing("category", checkdata.LibraryPropertiesSchemaValidationResult()[compliancelevel.Specification], configuration.SchemasPath()) {
Expand Down Expand Up @@ -731,8 +737,9 @@ func LibraryPropertiesCategoryFieldUncategorized() (result checkresult.Type, out

// LibraryPropertiesUrlFieldMissing checks for missing library.properties "url" field.
func LibraryPropertiesUrlFieldMissing() (result checkresult.Type, output string) {
if checkdata.LibraryPropertiesLoadError() != nil {
return checkresult.NotRun, "Couldn't load library.properties"
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
if !shouldRun {
return checkresult.NotRun, reason
}

if schema.RequiredPropertyMissing("url", checkdata.LibraryPropertiesSchemaValidationResult()[compliancelevel.Specification], configuration.SchemasPath()) {
Expand Down Expand Up @@ -785,8 +792,9 @@ func LibraryPropertiesUrlFieldDeadLink() (result checkresult.Type, output string

// LibraryPropertiesArchitecturesFieldMissing checks for missing library.properties "architectures" field.
func LibraryPropertiesArchitecturesFieldMissing() (result checkresult.Type, output string) {
if checkdata.LibraryPropertiesLoadError() != nil {
return checkresult.NotRun, "Couldn't load library.properties"
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
if !shouldRun {
return checkresult.NotRun, reason
}

if schema.RequiredPropertyMissing("architectures", checkdata.LibraryPropertiesSchemaValidationResult()[compliancelevel.Specification], configuration.SchemasPath()) {
Expand Down Expand Up @@ -841,12 +849,14 @@ func LibraryPropertiesDependsFieldNotInIndex() (result checkresult.Type, output
return checkresult.NotRun, "Field not present"
}

dependencies, err := properties.SplitQuotedString(depends, "", false)
if err != nil {
panic(err)
}
dependencies := strings.Split(depends, ",")

dependenciesNotInIndex := []string{}
for _, dependency := range dependencies {
dependency = strings.TrimSpace(dependency)
if dependency == "" {
continue
}
logrus.Tracef("Checking if dependency %s is in index.", dependency)
if !nameInLibraryManagerIndex(dependency) {
dependenciesNotInIndex = append(dependenciesNotInIndex, dependency)
Expand Down Expand Up @@ -923,12 +933,13 @@ func LibraryPropertiesIncludesFieldItemNotFound() (result checkresult.Type, outp
return checkresult.NotRun, "Field not present"
}

includesList, err := properties.SplitQuotedString(includes, "", false)
if err != nil {
panic(err)
}
includesList := strings.Split(includes, ",")

findInclude := func(include string) bool {
include = strings.TrimSpace(include)
if include == "" {
return true
}
for _, header := range checkdata.SourceHeaders() {
logrus.Tracef("Comparing include %s with header file %s", include, header)
if include == header {
Expand Down Expand Up @@ -1281,7 +1292,7 @@ func spellCheckLibraryPropertiesFieldValue(fieldName string) (result checkresult
}

replaced, diff := checkdata.MisspelledWordsReplacer().Replace(fieldValue)
if diff != nil {
if len(diff) > 0 {
return checkresult.Fail, replaced
}

Expand Down Expand Up @@ -1316,3 +1327,15 @@ func nameInLibraryManagerIndex(name string) bool {

return false
}

func runRequiredLibraryPropertiesFieldCheck() (bool, string) {
if checkdata.LibraryPropertiesLoadError() != nil {
return false, "Couldn't load library.properties"
}

if checkdata.LoadedLibrary().IsLegacy {
return false, "Library has legacy format"
}

return true, ""
}
3 changes: 3 additions & 0 deletions check/checkfunctions/library_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ func TestLibraryPropertiesSentenceFieldSpellCheck(t *testing.T) {
{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
{"Not defined", "MissingFields", checkresult.NotRun, ""},
{"Misspelled word", "MisspelledSentenceParagraphValue", checkresult.Fail, "^grill broccoli now$"},
{"Non-nil diff but no typos", "SpuriousMisspelledSentenceParagraphValue", checkresult.Pass, ""},
{"Correct spelling", "Recursive", checkresult.Pass, ""},
}

Expand All @@ -275,6 +276,7 @@ func TestLibraryPropertiesParagraphFieldSpellCheck(t *testing.T) {
{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
{"Not defined", "MissingFields", checkresult.NotRun, ""},
{"Misspelled word", "MisspelledSentenceParagraphValue", checkresult.Fail, "^There is a zebra$"},
{"Non-nil diff but no typos", "SpuriousMisspelledSentenceParagraphValue", checkresult.Pass, ""},
{"Correct spelling", "Recursive", checkresult.Pass, ""},
}

Expand Down Expand Up @@ -307,6 +309,7 @@ func TestLibraryPropertiesDependsFieldNotInIndex(t *testing.T) {
{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
{"Dependency not in index", "DependsNotIndexed", checkresult.Fail, "^NotIndexed$"},
{"Dependency in index", "DependsIndexed", checkresult.Pass, ""},
{"Depends field empty", "DependsEmpty", checkresult.Pass, ""},
{"No depends", "NoDepends", checkresult.NotRun, ""},
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
name=DependsEmpty
version=1.0.0
author=Cristian Maglie <c.maglie@example.com>, Pippo Pluto <pippo@example.com>
maintainer=Cristian Maglie <c.maglie@example.com>
sentence=A library that makes coding a web server a breeze.
paragraph=Supports HTTP1.1 and you can do GET and POST.
category=Communication
url=http://example.com/
architectures=avr
depends=
Empty file.
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ paragraph=Supports HTTP1.1 and you can do GET and POST.
category=Communication
url=http://example.com/
architectures=avr
depends=Servo
depends=Servo, , Adafruit NeoPixel
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ paragraph=Supports HTTP1.1 and you can do GET and POST.
category=Communication
url=http://example.com/
architectures=avr
includes=Nonexistent.h
includes=Nonexistent.h,MissingIncludes.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
name=SpuriousMisspelledSentenceParagraphValue
version=1.0.0
author=Cristian Maglie <c.maglie@example.com>, Pippo Pluto <pippo@example.com>
maintainer=Cristian Maglie <c.maglie@example.com>
sentence=Minimal bit bang send serial 115200 or 38400 baud for 1 MHz or 230400 baud for 16 MHz clock. Perfect
paragraph=Minimal bit bang send serial 115200 or 38400 baud for 1 MHz or 230400 baud for 16 MHz clock. Perfect
category=Communication
url=http://example.com/
architectures=avr
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@
"$ref": "#/definitions/propertiesObjects/version/base/object"
},
{
"$ref": "general-definitions-schema.json#/definitions/patternObjects/semver"
"$ref": "general-definitions-schema.json#/definitions/patternObjects/relaxedSemver"
}
]
}
Expand All @@ -154,7 +154,10 @@
"object": {
"allOf": [
{
"$ref": "#/definitions/propertiesObjects/version/specification/object"
"$ref": "#/definitions/propertiesObjects/version/base/object"
},
{
"$ref": "general-definitions-schema.json#/definitions/patternObjects/semver"
}
]
}
Expand Down Expand Up @@ -490,7 +493,7 @@
"base": {
"definitions": {
"patternObject": {
"pattern": "^(([a-zA-Z][a-zA-Z0-9 _\\.\\-,]*)|([0-9][a-zA-Z0-9 _\\.\\-]*[a-zA-Z][a-zA-Z0-9 _\\.\\-,]*))$"
"pattern": "^(([a-zA-Z][a-zA-Z0-9 _\\.\\-,]*)|([0-9][a-zA-Z0-9 _\\.\\-]*[a-zA-Z][a-zA-Z0-9 _\\.\\-,]*))*$"
}
},
"object": {
Expand Down
17 changes: 13 additions & 4 deletions project/library/libraryproperties/librarypropertiesschemas_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,11 @@ func TestPropertiesNamePattern(t *testing.T) {
{"Disallowed character", "-foo", "/patternObjects/allowedCharacters", compliancelevel.Specification, assert.True},
{"Disallowed character", "-foo", "/patternObjects/allowedCharacters", compliancelevel.Strict, assert.True},

// The "minLength" schema will enforce the minimum length, so this is not the responsibility of the pattern schema.
{"Empty", "", "/patternObjects/allowedCharacters", compliancelevel.Permissive, assert.False},
{"Empty", "", "/patternObjects/allowedCharacters", compliancelevel.Specification, assert.False},
{"Empty", "", "/patternObjects/allowedCharacters", compliancelevel.Strict, assert.False},

{"Starts with arduino", "arduinofoo", "/patternObjects/notStartsWithArduino", compliancelevel.Permissive, assert.False},
{"Starts with arduino", "arduinofoo", "/patternObjects/notStartsWithArduino", compliancelevel.Specification, assert.True},
{"Starts with arduino", "arduinofoo", "/patternObjects/notStartsWithArduino", compliancelevel.Strict, assert.True},
Expand Down Expand Up @@ -314,11 +319,11 @@ func TestPropertiesVersionPattern(t *testing.T) {
{"vX.Y.Z", "v1.0.0", compliancelevel.Strict, assert.True},

{"X.Y", "1.0", compliancelevel.Permissive, assert.False},
{"X.Y", "1.0", compliancelevel.Specification, assert.True},
{"X.Y", "1.0", compliancelevel.Specification, assert.False},
{"X.Y", "1.0", compliancelevel.Strict, assert.True},

{"X", "1", compliancelevel.Permissive, assert.False},
{"X", "1", compliancelevel.Specification, assert.True},
{"X", "1", compliancelevel.Specification, assert.False},
{"X", "1", compliancelevel.Strict, assert.True},
}

Expand Down Expand Up @@ -368,8 +373,12 @@ func TestPropertiesUrlFormat(t *testing.T) {
func TestPropertiesDependsPattern(t *testing.T) {
testTables := []propertyValueTestTable{
{"Invalid characters", "-foo", compliancelevel.Permissive, assert.True},
{"Invalid characters", "-foo", compliancelevel.Permissive, assert.True},
{"Invalid characters", "-foo", compliancelevel.Permissive, assert.True},
{"Invalid characters", "-foo", compliancelevel.Specification, assert.True},
{"Invalid characters", "-foo", compliancelevel.Strict, assert.True},

{"Empty", "", compliancelevel.Permissive, assert.False},
{"Empty", "", compliancelevel.Specification, assert.False},
{"Empty", "", compliancelevel.Strict, assert.False},
}

checkPropertyPatternMismatch("depends", testTables, t)
Expand Down
2 changes: 1 addition & 1 deletion result/result.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func (results *Type) Record(checkedProject project.Type, checkConfiguration chec
summaryText := fmt.Sprintf("Check %s result: %s\n", checkConfiguration.ID, checkResult)

checkMessage := ""
if checkLevel == checklevel.Error {
if checkResult == checkresult.Fail {
checkMessage = message(checkConfiguration.MessageTemplate, checkOutput)
} else {
// Checks may provide an explanation for their non-fail result.
Expand Down
1 change: 0 additions & 1 deletion result/result_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ func TestInitialize(t *testing.T) {
require.Nil(t, err)
var results Type
results.Initialize()
fmt.Printf("paths: %s", configuration.TargetPaths())
assert.Equal(t, paths.NewPathList(workingDirectoryPath), results.Configuration.Paths)
assert.Equal(t, projecttype.Sketch.String(), results.Configuration.ProjectType)
assert.False(t, results.Configuration.Recursive)
Expand Down