Skip to content

Commit 63844cd

Browse files
per1234per1234
authored
Merge pull request #96 from arduino/per1234/fix-checks
Fix bugs in check functions
2 parents 74200ad + be24c8e commit 63844cd

File tree

12 files changed

+96
-40
lines changed

12 files changed

+96
-40
lines changed

Diff for: ‎check/checkfunctions/library.go

+52-29
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@ import (
3333
"github.com/arduino/arduino-check/project/sketch"
3434
"github.com/arduino/arduino-cli/arduino/libraries"
3535
"github.com/arduino/arduino-cli/arduino/utils"
36-
"github.com/arduino/go-properties-orderedmap"
3736
"github.com/go-git/go-git/v5"
3837
"github.com/go-git/go-git/v5/plumbing"
3938
"github.com/go-git/go-git/v5/plumbing/object"
@@ -111,8 +110,9 @@ func RedundantLibraryProperties() (result checkresult.Type, output string) {
111110

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

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

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

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

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

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

482483
// LibraryPropertiesAuthorFieldMissing checks for missing library.properties "author" field.
483484
func LibraryPropertiesAuthorFieldMissing() (result checkresult.Type, output string) {
484-
if checkdata.LibraryPropertiesLoadError() != nil {
485-
return checkresult.NotRun, "Couldn't load library.properties"
485+
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
486+
if !shouldRun {
487+
return checkresult.NotRun, reason
486488
}
487489

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

511513
// LibraryPropertiesMaintainerFieldMissing checks for missing library.properties "maintainer" field.
512514
func LibraryPropertiesMaintainerFieldMissing() (result checkresult.Type, output string) {
513-
if checkdata.LibraryPropertiesLoadError() != nil {
514-
return checkresult.NotRun, "Couldn't load library.properties"
515+
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
516+
if !shouldRun {
517+
return checkresult.NotRun, reason
515518
}
516519

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

614617
// LibraryPropertiesSentenceFieldMissing checks for missing library.properties "sentence" field.
615618
func LibraryPropertiesSentenceFieldMissing() (result checkresult.Type, output string) {
616-
if checkdata.LibraryPropertiesLoadError() != nil {
617-
return checkresult.NotRun, "Couldn't load library.properties"
619+
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
620+
if !shouldRun {
621+
return checkresult.NotRun, reason
618622
}
619623

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

648652
// LibraryPropertiesParagraphFieldMissing checks for missing library.properties "paragraph" field.
649653
func LibraryPropertiesParagraphFieldMissing() (result checkresult.Type, output string) {
650-
if checkdata.LibraryPropertiesLoadError() != nil {
651-
return checkresult.NotRun, "Couldn't load library.properties"
654+
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
655+
if !shouldRun {
656+
return checkresult.NotRun, reason
652657
}
653658

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

684689
// LibraryPropertiesCategoryFieldMissing checks for missing library.properties "category" field.
685690
func LibraryPropertiesCategoryFieldMissing() (result checkresult.Type, output string) {
686-
if checkdata.LibraryPropertiesLoadError() != nil {
687-
return checkresult.NotRun, "Couldn't load library.properties"
691+
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
692+
if !shouldRun {
693+
return checkresult.NotRun, reason
688694
}
689695

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

732738
// LibraryPropertiesUrlFieldMissing checks for missing library.properties "url" field.
733739
func LibraryPropertiesUrlFieldMissing() (result checkresult.Type, output string) {
734-
if checkdata.LibraryPropertiesLoadError() != nil {
735-
return checkresult.NotRun, "Couldn't load library.properties"
740+
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
741+
if !shouldRun {
742+
return checkresult.NotRun, reason
736743
}
737744

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

786793
// LibraryPropertiesArchitecturesFieldMissing checks for missing library.properties "architectures" field.
787794
func LibraryPropertiesArchitecturesFieldMissing() (result checkresult.Type, output string) {
788-
if checkdata.LibraryPropertiesLoadError() != nil {
789-
return checkresult.NotRun, "Couldn't load library.properties"
795+
shouldRun, reason := runRequiredLibraryPropertiesFieldCheck()
796+
if !shouldRun {
797+
return checkresult.NotRun, reason
790798
}
791799

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

844-
dependencies, err := properties.SplitQuotedString(depends, "", false)
845-
if err != nil {
846-
panic(err)
847-
}
852+
dependencies := strings.Split(depends, ",")
853+
848854
dependenciesNotInIndex := []string{}
849855
for _, dependency := range dependencies {
856+
dependency = strings.TrimSpace(dependency)
857+
if dependency == "" {
858+
continue
859+
}
850860
logrus.Tracef("Checking if dependency %s is in index.", dependency)
851861
if !nameInLibraryManagerIndex(dependency) {
852862
dependenciesNotInIndex = append(dependenciesNotInIndex, dependency)
@@ -923,12 +933,13 @@ func LibraryPropertiesIncludesFieldItemNotFound() (result checkresult.Type, outp
923933
return checkresult.NotRun, "Field not present"
924934
}
925935

926-
includesList, err := properties.SplitQuotedString(includes, "", false)
927-
if err != nil {
928-
panic(err)
929-
}
936+
includesList := strings.Split(includes, ",")
930937

931938
findInclude := func(include string) bool {
939+
include = strings.TrimSpace(include)
940+
if include == "" {
941+
return true
942+
}
932943
for _, header := range checkdata.SourceHeaders() {
933944
logrus.Tracef("Comparing include %s with header file %s", include, header)
934945
if include == header {
@@ -1281,7 +1292,7 @@ func spellCheckLibraryPropertiesFieldValue(fieldName string) (result checkresult
12811292
}
12821293

12831294
replaced, diff := checkdata.MisspelledWordsReplacer().Replace(fieldValue)
1284-
if diff != nil {
1295+
if len(diff) > 0 {
12851296
return checkresult.Fail, replaced
12861297
}
12871298

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

13171328
return false
13181329
}
1330+
1331+
func runRequiredLibraryPropertiesFieldCheck() (bool, string) {
1332+
if checkdata.LibraryPropertiesLoadError() != nil {
1333+
return false, "Couldn't load library.properties"
1334+
}
1335+
1336+
if checkdata.LoadedLibrary().IsLegacy {
1337+
return false, "Library has legacy format"
1338+
}
1339+
1340+
return true, ""
1341+
}

Diff for: ‎check/checkfunctions/library_test.go

+3
Original file line numberDiff line numberDiff line change
@@ -264,6 +264,7 @@ func TestLibraryPropertiesSentenceFieldSpellCheck(t *testing.T) {
264264
{"Unable to load", "InvalidLibraryProperties", checkresult.NotRun, ""},
265265
{"Not defined", "MissingFields", checkresult.NotRun, ""},
266266
{"Misspelled word", "MisspelledSentenceParagraphValue", checkresult.Fail, "^grill broccoli now$"},
267+
{"Non-nil diff but no typos", "SpuriousMisspelledSentenceParagraphValue", checkresult.Pass, ""},
267268
{"Correct spelling", "Recursive", checkresult.Pass, ""},
268269
}
269270

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

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

Diff for: ‎check/checkfunctions/testdata/libraries/DependsEmpty/library.properties

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
name=DependsEmpty
2+
version=1.0.0
3+
author=Cristian Maglie <[email protected]>, Pippo Pluto <[email protected]>
4+
maintainer=Cristian Maglie <[email protected]>
5+
sentence=A library that makes coding a web server a breeze.
6+
paragraph=Supports HTTP1.1 and you can do GET and POST.
7+
category=Communication
8+
url=http://example.com/
9+
architectures=avr
10+
depends=

Diff for: ‎check/checkfunctions/testdata/libraries/DependsEmpty/src/DependsEmpty.h

Whitespace-only changes.

Diff for: ‎check/checkfunctions/testdata/libraries/DependsIndexed/library.properties

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,4 +7,4 @@ paragraph=Supports HTTP1.1 and you can do GET and POST.
77
category=Communication
88
url=http://example.com/
99
architectures=avr
10-
depends=Servo
10+
depends=Servo, , Adafruit NeoPixel

Diff for: ‎check/checkfunctions/testdata/libraries/MissingIncludes/library.properties

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,4 +7,4 @@ paragraph=Supports HTTP1.1 and you can do GET and POST.
77
category=Communication
88
url=http://example.com/
99
architectures=avr
10-
includes=Nonexistent.h
10+
includes=Nonexistent.h,MissingIncludes.h

Diff for: ‎check/checkfunctions/testdata/libraries/SpuriousMisspelledSentenceParagraphValue/library.properties

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
name=SpuriousMisspelledSentenceParagraphValue
2+
version=1.0.0
3+
author=Cristian Maglie <[email protected]>, Pippo Pluto <[email protected]>
4+
maintainer=Cristian Maglie <[email protected]>
5+
sentence=Minimal bit bang send serial 115200 or 38400 baud for 1 MHz or 230400 baud for 16 MHz clock. Perfect
6+
paragraph=Minimal bit bang send serial 115200 or 38400 baud for 1 MHz or 230400 baud for 16 MHz clock. Perfect
7+
category=Communication
8+
url=http://example.com/
9+
architectures=avr

Diff for: ‎check/checkfunctions/testdata/libraries/SpuriousMisspelledSentenceParagraphValue/src/SpuriousMisspelledSentenceParagraphValue.h

Whitespace-only changes.

Diff for: ‎etc/schemas/arduino-library-properties-definitions-schema.json

+6-3
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@
145145
"$ref": "#/definitions/propertiesObjects/version/base/object"
146146
},
147147
{
148-
"$ref": "general-definitions-schema.json#/definitions/patternObjects/semver"
148+
"$ref": "general-definitions-schema.json#/definitions/patternObjects/relaxedSemver"
149149
}
150150
]
151151
}
@@ -154,7 +154,10 @@
154154
"object": {
155155
"allOf": [
156156
{
157-
"$ref": "#/definitions/propertiesObjects/version/specification/object"
157+
"$ref": "#/definitions/propertiesObjects/version/base/object"
158+
},
159+
{
160+
"$ref": "general-definitions-schema.json#/definitions/patternObjects/semver"
158161
}
159162
]
160163
}
@@ -490,7 +493,7 @@
490493
"base": {
491494
"definitions": {
492495
"patternObject": {
493-
"pattern": "^(([a-zA-Z][a-zA-Z0-9 _\\.\\-,]*)|([0-9][a-zA-Z0-9 _\\.\\-]*[a-zA-Z][a-zA-Z0-9 _\\.\\-,]*))$"
496+
"pattern": "^(([a-zA-Z][a-zA-Z0-9 _\\.\\-,]*)|([0-9][a-zA-Z0-9 _\\.\\-]*[a-zA-Z][a-zA-Z0-9 _\\.\\-,]*))*$"
494497
}
495498
},
496499
"object": {

Diff for: ‎project/library/libraryproperties/librarypropertiesschemas_test.go

+13-4
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,11 @@ func TestPropertiesNamePattern(t *testing.T) {
283283
{"Disallowed character", "-foo", "/patternObjects/allowedCharacters", compliancelevel.Specification, assert.True},
284284
{"Disallowed character", "-foo", "/patternObjects/allowedCharacters", compliancelevel.Strict, assert.True},
285285

286+
// The "minLength" schema will enforce the minimum length, so this is not the responsibility of the pattern schema.
287+
{"Empty", "", "/patternObjects/allowedCharacters", compliancelevel.Permissive, assert.False},
288+
{"Empty", "", "/patternObjects/allowedCharacters", compliancelevel.Specification, assert.False},
289+
{"Empty", "", "/patternObjects/allowedCharacters", compliancelevel.Strict, assert.False},
290+
286291
{"Starts with arduino", "arduinofoo", "/patternObjects/notStartsWithArduino", compliancelevel.Permissive, assert.False},
287292
{"Starts with arduino", "arduinofoo", "/patternObjects/notStartsWithArduino", compliancelevel.Specification, assert.True},
288293
{"Starts with arduino", "arduinofoo", "/patternObjects/notStartsWithArduino", compliancelevel.Strict, assert.True},
@@ -314,11 +319,11 @@ func TestPropertiesVersionPattern(t *testing.T) {
314319
{"vX.Y.Z", "v1.0.0", compliancelevel.Strict, assert.True},
315320

316321
{"X.Y", "1.0", compliancelevel.Permissive, assert.False},
317-
{"X.Y", "1.0", compliancelevel.Specification, assert.True},
322+
{"X.Y", "1.0", compliancelevel.Specification, assert.False},
318323
{"X.Y", "1.0", compliancelevel.Strict, assert.True},
319324

320325
{"X", "1", compliancelevel.Permissive, assert.False},
321-
{"X", "1", compliancelevel.Specification, assert.True},
326+
{"X", "1", compliancelevel.Specification, assert.False},
322327
{"X", "1", compliancelevel.Strict, assert.True},
323328
}
324329

@@ -368,8 +373,12 @@ func TestPropertiesUrlFormat(t *testing.T) {
368373
func TestPropertiesDependsPattern(t *testing.T) {
369374
testTables := []propertyValueTestTable{
370375
{"Invalid characters", "-foo", compliancelevel.Permissive, assert.True},
371-
{"Invalid characters", "-foo", compliancelevel.Permissive, assert.True},
372-
{"Invalid characters", "-foo", compliancelevel.Permissive, assert.True},
376+
{"Invalid characters", "-foo", compliancelevel.Specification, assert.True},
377+
{"Invalid characters", "-foo", compliancelevel.Strict, assert.True},
378+
379+
{"Empty", "", compliancelevel.Permissive, assert.False},
380+
{"Empty", "", compliancelevel.Specification, assert.False},
381+
{"Empty", "", compliancelevel.Strict, assert.False},
373382
}
374383

375384
checkPropertyPatternMismatch("depends", testTables, t)

Diff for: ‎result/result.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ func (results *Type) Record(checkedProject project.Type, checkConfiguration chec
102102
summaryText := fmt.Sprintf("Check %s result: %s\n", checkConfiguration.ID, checkResult)
103103

104104
checkMessage := ""
105-
if checkLevel == checklevel.Error {
105+
if checkResult == checkresult.Fail {
106106
checkMessage = message(checkConfiguration.MessageTemplate, checkOutput)
107107
} else {
108108
// Checks may provide an explanation for their non-fail result.

Diff for: ‎result/result_test.go

-1
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,6 @@ func TestInitialize(t *testing.T) {
5555
require.Nil(t, err)
5656
var results Type
5757
results.Initialize()
58-
fmt.Printf("paths: %s", configuration.TargetPaths())
5958
assert.Equal(t, paths.NewPathList(workingDirectoryPath), results.Configuration.Paths)
6059
assert.Equal(t, projecttype.Sketch.String(), results.Configuration.ProjectType)
6160
assert.False(t, results.Configuration.Recursive)

0 commit comments

Comments
 (0)