Structural modifications in error checks

-- Added file name display in error messages
- Structured the tests in respective files

Signed-off-by: specter25 <ujjwalcoding012@gmail.com>

8 files changed, 53 insertions, 46 deletions

diff --git a/tvloader/parser2v1/parse_creation_info.go b/tvloader/parser2v1/parse_creation_info.go
index c492884..dd9a004 100644
--- a/tvloader/parser2v1/parse_creation_info.go
+++ b/tvloader/parser2v1/parse_creation_info.go
@@ -78,7 +78,7 @@ func (parser *tvParser2_1) parsePairFromCreationInfo2_1(tag string, value string
 	case "PackageName":
 		// Error if last file does not has FileSPDXId
 		if parser.file != nil && parser.file.FileSPDXIdentifier == spdx.ElementID("") {
-			return fmt.Errorf("invalid file without a package SPDX identifier")
+			return fmt.Errorf("file with FileName %s does not have SPDX identifier", parser.file.FileName)
 		}
 		parser.st = psPackage2_1
 		parser.pkg = &spdx.Package2_1{
diff --git a/tvloader/parser2v1/parse_file.go b/tvloader/parser2v1/parse_file.go
index 772e36e..7347384 100644
--- a/tvloader/parser2v1/parse_file.go
+++ b/tvloader/parser2v1/parse_file.go
@@ -20,7 +20,7 @@ func (parser *tvParser2_1) parsePairFromFile2_1(tag string, value string) error
 	case "FileName":
 		// check if the previous file contained a spdxId or not
 		if parser.file != nil && parser.file.FileSPDXIdentifier == nullSpdxElementId2_1 {
-			return fmt.Errorf("invalid file without a file SPDX identifier")
+			return fmt.Errorf("file with FileName %s does not have SPDX identifier", parser.file.FileName)
 		}
 		parser.file = &spdx.File2_1{}
 		parser.file.FileName = value
@@ -28,7 +28,7 @@ func (parser *tvParser2_1) parsePairFromFile2_1(tag string, value string) error
 	case "PackageName":
 		// check if the previous file contained a spdxId or not
 		if parser.file != nil && parser.file.FileSPDXIdentifier == nullSpdxElementId2_1 {
-			return fmt.Errorf("invalid file without a file SPDX identifier")
+			return fmt.Errorf("file with FileName %s does not have SPDX identifier", parser.file.FileName)
 		}
 		parser.st = psPackage2_1
 		parser.file = nil
diff --git a/tvloader/parser2v1/parse_file_test.go b/tvloader/parser2v1/parse_file_test.go
index 481d577..e302a53 100644
--- a/tvloader/parser2v1/parse_file_test.go
+++ b/tvloader/parser2v1/parse_file_test.go
@@ -5,7 +5,6 @@ import (
 	"testing"
 
 	"github.com/spdx/tools-golang/spdx"
-	"github.com/spdx/tools-golang/tvloader/reader"
 )
 
 // ===== Parser file section state change tests =====
@@ -887,44 +886,34 @@ func TestParser2_1FailsIfArtifactURIBeforeArtifactName(t *testing.T) {
 }
 
 func TestParser2_1FilesWithoutSpdxIdThrowError(t *testing.T) {
-	// case 1
-	// Last unpackaged file no packages in doc
-	// Last file of last package in the doc
-	tvPairs := []reader.TagValuePair{
-		{Tag: "SPDXVersion", Value: "SPDX-2.1"},
-		{Tag: "DataLicense", Value: "CC0-1.0"},
-		{Tag: "SPDXID", Value: "SPDXRef-DOCUMENT"},
-		{Tag: "FileName", Value: "f1"},
-	}
-	_, err := ParseTagValues(tvPairs)
-	if err == nil {
-		t.Errorf("files withoutSpdx Identifiers getting accepted")
+	// case 1 : The previous file (packaged or unpackaged does not contain spdxID)
+	parser1 := tvParser2_1{
+		doc:  &spdx.Document2_1{Packages: map[spdx.ElementID]*spdx.Package2_1{}},
+		st:   psFile2_1,
+		file: &spdx.File2_1{FileName: "FileName"},
 	}
 
-	// case 2 : The previous file (packaged or unpackaged does not contain spdxID)
-	tvPairs = append(tvPairs, reader.TagValuePair{Tag: "FileName", Value: "f2"})
-	_, err = ParseTagValues(tvPairs)
+	err := parser1.parsePair2_1("FileName", "f2")
 	if err == nil {
-		t.Errorf("%s", err)
+		t.Errorf("files withoutSpdx Identifiers getting accepted")
 	}
 
-	// case 3 : Invalid file with snippet
-	// Last unpackaged file before the packges start
+	// case 2 : Invalid file with snippet
+	// Last unpackaged file before the snippet start
 	// Last file of a package and New package starts
 	sid1 := spdx.ElementID("s1")
-	parser := tvParser2_1{
+	parser2 := tvParser2_1{
 		doc: &spdx.Document2_1{},
 		st:  psCreationInfo2_1,
 	}
 	fileName := "f2.txt"
-	_ = parser.parsePair2_1("FileName", fileName)
-	_ = parser.parsePair2_1("SnippetSPDXID", string(sid1))
-	err = parser.parsePair2_1("PackageName", "p2")
+	_ = parser2.parsePair2_1("FileName", fileName)
+	err = parser2.parsePair2_1("SnippetSPDXID", string(sid1))
 	if err == nil {
 		t.Errorf("files withoutSpdx Identifiers getting accepted")
 	}
 
-	// case 4 : Invalid File without snippets
+	// case 3 : Invalid File without snippets
 	// Last unpackaged file before the packges start
 	// Last file of a package and New package starts
 	parser3 := tvParser2_1{
diff --git a/tvloader/parser2v1/parse_package.go b/tvloader/parser2v1/parse_package.go
index 2ac355a..eb96f4c 100644
--- a/tvloader/parser2v1/parse_package.go
+++ b/tvloader/parser2v1/parse_package.go
@@ -22,7 +22,7 @@ func (parser *tvParser2_1) parsePairFromPackage2_1(tag string, value string) err
 		if parser.pkg == nil || parser.pkg.PackageName != "" {
 			// check if the previous package contained a spdxId or not
 			if parser.pkg != nil && parser.pkg.PackageSPDXIdentifier == nullSpdxElementId2_1 {
-				return fmt.Errorf("invalid package without a package SPDX identifier")
+				return fmt.Errorf("package with PackageName %s does not have SPDX identifier", parser.pkg.PackageName)
 			}
 			parser.pkg = &spdx.Package2_1{
 				FilesAnalyzed:             true,
diff --git a/tvloader/parser2v1/parse_package_test.go b/tvloader/parser2v1/parse_package_test.go
index 3aa0ddb..e43727b 100644
--- a/tvloader/parser2v1/parse_package_test.go
+++ b/tvloader/parser2v1/parse_package_test.go
@@ -5,7 +5,6 @@ import (
 	"testing"
 
 	"github.com/spdx/tools-golang/spdx"
-	"github.com/spdx/tools-golang/tvloader/reader"
 )
 
 // ===== Parser package section state change tests =====
@@ -1088,20 +1087,9 @@ func TestParser2_1PackageWithoutSpdxIdentifierThrowsError(t *testing.T) {
 		t.Errorf("expected 1 package, got %d", len(parser.doc.Packages))
 	}
 
-	// Case 2: Checks the Last package
 	pkgName := "p2"
 	err := parser.parsePair2_1("PackageName", pkgName)
 	if err == nil {
 		t.Errorf("packages withoutSpdx Identifiers getting accepted")
 	}
-	tvPairs := []reader.TagValuePair{
-		{Tag: "SPDXVersion", Value: "SPDX-2.1"},
-		{Tag: "DataLicense", Value: "CC0-1.0"},
-		{Tag: "SPDXID", Value: "SPDXRef-DOCUMENT"},
-		{Tag: "PackageName", Value: "p1"},
-	}
-	_, err = ParseTagValues(tvPairs)
-	if err == nil {
-		t.Errorf("packages withoutSpdx Identifiers getting accepted")
-	}
 }
diff --git a/tvloader/parser2v1/parse_snippet.go b/tvloader/parser2v1/parse_snippet.go
index f44c964..b8aafad 100644
--- a/tvloader/parser2v1/parse_snippet.go
+++ b/tvloader/parser2v1/parse_snippet.go
@@ -13,6 +13,10 @@ func (parser *tvParser2_1) parsePairFromSnippet2_1(tag string, value string) err
 	switch tag {
 	// tag for creating new snippet section
 	case "SnippetSPDXID":
+		// check here whether the file containe
+		if parser.file != nil && parser.file.FileSPDXIdentifier == nullSpdxElementId2_1 {
+			return fmt.Errorf("file with FileName %s does not have SPDX identifier", parser.file.FileName)
+		}
 		parser.snippet = &spdx.Snippet2_1{}
 		eID, err := extractElementID(value)
 		if err != nil {
@@ -34,10 +38,6 @@ func (parser *tvParser2_1) parsePairFromSnippet2_1(tag string, value string) err
 	// tag for creating new package section and going back to parsing Package
 	case "PackageName":
 		parser.st = psPackage2_1
-		// check here whether the last file of the previous package contained the FileSpdxIdentifier
-		if parser.file != nil && parser.file.FileSPDXIdentifier == nullSpdxElementId2_1 {
-			return fmt.Errorf("invalid file without a file SPDX identifier")
-		}
 		parser.file = nil
 		parser.snippet = nil
 		return parser.parsePairFromPackage2_1(tag, value)
diff --git a/tvloader/parser2v1/parser.go b/tvloader/parser2v1/parser.go
index 6504474..f4a5ae1 100644
--- a/tvloader/parser2v1/parser.go
+++ b/tvloader/parser2v1/parser.go
@@ -21,10 +21,10 @@ func ParseTagValues(tvs []reader.TagValuePair) (*spdx.Document2_1, error) {
 		}
 	}
 	if parser.file != nil && parser.file.FileSPDXIdentifier == nullSpdxElementId2_1 {
-		return nil, fmt.Errorf("invalid file without a file SPDX identifier")
+		return nil, fmt.Errorf("file with FileName %s does not have SPDX identifier", parser.file.FileName)
 	}
 	if parser.pkg != nil && parser.pkg.PackageSPDXIdentifier == nullSpdxElementId2_1 {
-		return nil, fmt.Errorf("invalid package without a package SPDX identifier")
+		return nil, fmt.Errorf("package with PackageName %s does not have SPDX identifier", parser.pkg.PackageName)
 	}
 
 	return parser.doc, nil
diff --git a/tvloader/parser2v1/parser_test.go b/tvloader/parser2v1/parser_test.go
index 4a61828..0678fd7 100644
--- a/tvloader/parser2v1/parser_test.go
+++ b/tvloader/parser2v1/parser_test.go
@@ -77,3 +77,33 @@ func TestParser2_1StartFailsToParseIfInInvalidState(t *testing.T) {
 		t.Errorf("expected non-nil error, got nil")
 	}
 }
+
+func TestParser2_1FilesWithoutSpdxIdThrowErrorAtCompleteParse(t *testing.T) {
+	// case
+	// Last unpackaged file no packages in doc
+	// Last file of last package in the doc
+	tvPairs := []reader.TagValuePair{
+		{Tag: "SPDXVersion", Value: "SPDX-2.1"},
+		{Tag: "DataLicense", Value: "CC0-1.0"},
+		{Tag: "SPDXID", Value: "SPDXRef-DOCUMENT"},
+		{Tag: "FileName", Value: "f1"},
+	}
+	_, err := ParseTagValues(tvPairs)
+	if err == nil {
+		t.Errorf("files withoutSpdx Identifiers getting accepted")
+	}
+}
+
+func TestParser2_1PackageWithoutSpdxIdThrowErrorAtCompleteParse(t *testing.T) {
+	// case : Checks the last package
+	tvPairs := []reader.TagValuePair{
+		{Tag: "SPDXVersion", Value: "SPDX-2.1"},
+		{Tag: "DataLicense", Value: "CC0-1.0"},
+		{Tag: "SPDXID", Value: "SPDXRef-DOCUMENT"},
+		{Tag: "PackageName", Value: "p1"},
+	}
+	_, err := ParseTagValues(tvPairs)
+	if err == nil {
+		t.Errorf("packages withoutSpdx Identifiers getting accepted")
+	}
+}