🌱 add csv import test #390

khareyash05 · 2023-06-13T02:40:51Z

Closes #388

Work Done as of now

Created a sample of data to be passed in samples.go
Initialized RichClient.Client
Uploaded CSV using FilePost
Get contents of /importsummaries
Compare Applications and Dependencies with the output of the API
Compare valid count with no. of Applications and Dependencies
Added checks for all routes
Download the CSV
Compare informations of CSV's

Signed-off-by: Yash Khare <[email protected]>

aufi

Nice start, Yash! Added few comments primary on naming. About extending the client.go with file upload, there is a PR #393 updating FileSend, let's see if it is useful also for this purpose once it gets merged (will review then).

Looking forward to update checking of Imports status and created resources to find all things needed that need to be tested there.

test/api/importCSV/api_test.go

test/api/importCSV/samples.go

test/api/importCSV/api_test.go

khareyash05 · 2023-06-14T14:05:02Z

Nice start, Yash! Added few comments primary on naming. About extending the client.go with file upload, there is a PR #393 updating FileSend, let's see if it is useful also for this purpose once it gets merged (will review then).

Looking forward to update checking of Imports status and created resources to find all things needed that need to be tested there.

Thanks Marek. Ya I went through the PR and seems to me that merging it will cause merge conflicts. Better to wait.
As for update checking I am figuring out a few things and will update them as soon as the it is done. Thank you for the review.

Signed-off-by: Yash Khare <[email protected]>

khareyash05 · 2023-06-16T12:03:59Z

Added comparison logic for applications and dependencies with the expected API output

aufi

Good progress, added few comments.

aufi · 2023-06-16T13:15:00Z

test/api/importcsv/samples.go

+var (
+	TestCases = []TestCase{
+		{
+			fileName: "template_application_import.csv",


Not sure about fileName vs. FileName.

Is there any specific reference to this name change? Because I found UploadCSV() using fileName, thatswhy named the same

aufi · 2023-06-16T13:16:38Z

test/api/importcsv/samples.go

+	TestCases = []TestCase{
+		{
+			fileName: "template_application_import.csv",
+			ExpectedApplications: []api.Application{


This array looks good, please go ahead and add Application fields data from the CSV.

So the data in this CSV is supposed to be equivalent to what is being described in the template_application_import.csv?

So the data in this CSV is supposed to be equivalent to what is being described in the template_application_import.csv?

Ya even I think so because we are checking that if all the entities are being uploaded or not thus have to check them all

aufi · 2023-06-16T14:27:08Z

test/api/importcsv/api_test.go

+			expectedApps := r.ExpectedApplications
+			expectedDeps := r.ExpectedDependencies
+			importList, ok := imports.([]interface{})
+			if ok {


Look to API source code, there is ImportSummary struct as well as Import struct which is a middle step between Import CSV and created Applications (https://github.com/konveyor/tackle2-hub/blob/main/api/import.go#L271-L289 and processed in https://github.com/konveyor/tackle2-hub/blob/main/importer/manager.go#L51).

Try to check Applications directly with application.List() and fields on ImportSummary like count of valid records imported.

Generaly, it is always good to check the source code and the feature that is being tested to find out how it works, identify what might make sense to be checked (in addition to fields of an Applications/Dependencies) and since go is a static language, we should utilize its advantages, so use interfaces/casting only in necessary situations.

Signed-off-by: Yash Khare <[email protected]>

aufi

Thanks for update! See comments inline + try follow similar conventions as in previous dependency test, e.g. when creating paths for API request, use consts from API package instead of hardcoding paths in string (details inline).

binding/client.go

aufi · 2023-06-26T12:51:10Z

binding/client.go

@@ -368,7 +369,7 @@ func (r *Client) BucketPut(source, destination string) (err error) {
 			if isDir {
 				err = r.putDir(part, source)
 			} else {
-				err = r.putFile(part, source)
+				err = r.loadFile(part, source)


Please correct me if I'm wrong, but the changes related to putFile -> loadFile methods doesn't seem to be needed since you created FilePost method.

We had actually changed the name of the function from putFile to loadFile to make it generic. Thus updated the same everywhere.

Understood, but this change came in time when there was another function calling the loadFile, which is gone since you added FilePost method (which is a better solution for this). So the putFile is called only from BucketPut, so I'd would not recommend changing it (as actually unrelated code to this test/PR).

test/api/importcsv/api_test.go

aufi · 2023-06-26T13:23:07Z

test/api/importcsv/api_test.go

+
+			// Check list of Dependencies.
+			importedDeps, _ := Dependency.List()
+			expectedDeps := r.ExpectedDependencies


Are these expectedDeps&expectedApps needed, wouldn't be better use r.Expected... variables?

Will do that.

test/api/importcsv/api_test.go

Signed-off-by: Yash Khare <[email protected]>

aufi · 2023-06-28T06:09:10Z

binding/client.go

@@ -368,7 +369,7 @@ func (r *Client) BucketPut(source, destination string) (err error) {
 			if isDir {
 				err = r.putDir(part, source)
 			} else {
-				err = r.putFile(part, source)
+				err = r.loadFile(part, source)


Understood, but this change came in time when there was another function calling the loadFile, which is gone since you added FilePost method (which is a better solution for this). So the putFile is called only from BucketPut, so I'd would not recommend changing it (as actually unrelated code to this test/PR).

aufi · 2023-06-28T06:18:39Z

test/api/importcsv/api_test.go

+				t.Errorf("Mismatch in number of imported Applications: Expected %d, Actual %d", len(expectedApps), len(gotApps))
+			} else {
+				for i, importedApp := range gotApps {
+					assert.FlatEqual(expectedApps[i].Name, importedApp.Name)


These assertions return true/false, but actually do nothing with the test result. Please look to the FlatEqual method body and to your previous PR to its usage (e.g. https://github.com/konveyor/tackle2-hub/blob/main/test/api/dependency/api_test.go#L34-L36)

In some cases, it might be better use == instead of FlatEqual function (expectedApps[i].Name, importedApp.Name => if expectedApps[i].Name != importedApp.Name {error...})

Will do that!

aufi · 2023-06-28T06:26:59Z

test/api/importcsv/api_test.go

+
+			// Upload CSV.
+			inputData := api.ImportSummary{}
+			assert.Must(t, Client.FilePost(api.SummariesRoot+"/upload", r.FileName, &inputData))


This is not the right way creating the path, please follow what you had in your previous PR https://github.com/konveyor/tackle2-hub/pull/375/files#diff-6ded4e5e60480daa4db7cf0740809d6b470d937e85d1b674b02777560fed1a38R25

Signed-off-by: Yash Khare <[email protected]>

aufi · 2023-06-28T15:49:27Z

migration/v2/model/dependency.go

 	To     *Application `gorm:"foreignKey:ToID;constraint:OnDelete:CASCADE"`
-	FromID uint         `gorm:"index"`
+	FromID uint         `gorm:"uniqueIndex:dependency_primary_key"`


Appreciate your effort on looking thi issue, but it cannot be in this PR with tests. Please create a new one referencing to reported issue and look on notes below.

the schema is versioned, it is needed create new version (or modify the latest one if it was not released yet - I'm not sure if it is possible now, let's see on the new PR)

I'm not sure about using "primary_key" as part of name of composite uniq key, would you consider a more descriptive name?

Surely ! Will address it in the next PR

Signed-off-by: Yash Khare <[email protected]>

aufi · 2023-06-29T14:41:02Z

test/api/importcsv/api_test.go

+			}
+
+			// Compare each value.
+			for i := 0; i < len(item1); i++ {


Happy to see such precise file content diff check (byte by byte), but it might be easier/more matching to the test/better to read both files data, cast []byte to string and compare it and in case of error, print both contents got and expected, that will be much easier to debug why a test potentialy failed.

Will work on that.Thanks Marek

Signed-off-by: Yash Khare <[email protected]>

aufi

Thanks for update Yash! Added few comments, please look on them.

aufi · 2023-06-30T12:01:47Z

binding/client.go

@@ -21,7 +21,7 @@ import (
 	pathlib "path"
 	"path/filepath"
 	"strings"
-	"time"
+	"time"	


Nit, but not needed change, please remove trailing spaces.

docs/test-api-matrix.md

test/api/importcsv/api_test.go

aufi · 2023-06-30T12:12:06Z

test/api/importcsv/api_test.go

+			assert.Must(t, Client.FilePost(api.UploadRoot, r.FileName, &inputData))
+
+			// Since uploading the CSV happens asynchronously we need to wait for the upload to check Applications and Dependencies.
+			time.Sleep(time.Second * 3)


I think we discussed a loop waiting for importsummary until it get process all records (e.g. sleep 1 sec, get importsummary and if valid+invalid count is equal to all application&dependencies count, break from the loop and continue).

This is not a blocker, just for info.

test/api/importcsv/api_test.go

aufi · 2023-06-30T12:31:34Z

test/api/importcsv/api_test.go

+						t.Errorf("Mismatch in name of import: Expected %s, Actual %s", expectedApps[j].BusinessService.Name, imp["businessService"])
+					}
+					j++
+				}


IIUC, shouldn't there be a second part of the condition - if outputImports has more records than expectedApps, the test should fail too. Same for dependencies few lines below.

outputimports consist of both applications and dependencies thus shouldn't be equal to length of sums?

If the CSV for this test would get one more line with an application, but the test (expected applications) was not changed (=> more imports type 1, but not changed len(expectedApps)), it looks to me to pass this part of the test anywa - that was the thing I didn't like. On the other hand there is a check of gotApps len() on line 44 and in this PR we don't have to do deeper rejection tests, so it is likely OK.

test/api/importcsv/api_test.go

Signed-off-by: Yash Khare <[email protected]>

aufi

Left few minor comments, but looks good to me.

docs/test-api-matrix.md

test/api/importcsv/api_test.go

aufi · 2023-07-03T09:48:49Z

test/api/importcsv/api_test.go

+						t.Errorf("Mismatch in name of import: Expected %s, Actual %s", expectedApps[j].BusinessService.Name, imp["businessService"])
+					}
+					j++
+				}


If the CSV for this test would get one more line with an application, but the test (expected applications) was not changed (=> more imports type 1, but not changed len(expectedApps)), it looks to me to pass this part of the test anywa - that was the thing I didn't like. On the other hand there is a check of gotApps len() on line 44 and in this PR we don't have to do deeper rejection tests, so it is likely OK.

Signed-off-by: Yash Khare <[email protected]>

aufi · 2023-07-03T11:26:05Z

Part of konveyor/operator#220

add csv import test

0ce61f5

Signed-off-by: Yash Khare <[email protected]>

khareyash05 changed the title ~~add csv import test~~ 🌱 add csv import test Jun 13, 2023

aufi self-assigned this Jun 13, 2023

khareyash05 added 4 commits June 13, 2023 17:23

add FilePost to Post CSV

b4acc5b

Signed-off-by: Yash Khare <[email protected]>

add get route

343a266

Signed-off-by: Yash Khare <[email protected]>

add comparison of id's

a3a9f50

Signed-off-by: Yash Khare <[email protected]>

updated variable names

723be07

Signed-off-by: Yash Khare <[email protected]>

aufi reviewed Jun 14, 2023

View reviewed changes

test/api/importCSV/api_test.go Outdated Show resolved Hide resolved

test/api/importCSV/api_test.go Outdated Show resolved Hide resolved

test/api/importCSV/samples.go Outdated Show resolved Hide resolved

test/api/importCSV/api_test.go Outdated Show resolved Hide resolved

khareyash05 added 2 commits June 14, 2023 22:30

Merge branch 'main' into test_csv_import

71bd769

Signed-off-by: Yash Khare <[email protected]>

added logic to compare apps and deps

4049dd4

Signed-off-by: Yash Khare <[email protected]>

khareyash05 requested a review from aufi June 16, 2023 11:58

khareyash05 marked this pull request as ready for review June 16, 2023 14:24

aufi reviewed Jun 16, 2023

View reviewed changes

khareyash05 marked this pull request as draft June 23, 2023 10:40

khareyash05 added 6 commits June 23, 2023 17:36

added Application and Dependency check

55ae1d1

Signed-off-by: Yash Khare <[email protected]>

Merge remote-tracking branch 'origin/main' into test_csv_import

e3a116f

added valid count check

606abcd

Signed-off-by: Yash Khare <[email protected]>

passed CI

69087da

Signed-off-by: Yash Khare <[email protected]>

refactor code to pass CI

69c4cec

Signed-off-by: Yash Khare <[email protected]>

added all import routes

0591e58

Signed-off-by: Yash Khare <[email protected]>

khareyash05 requested a review from aufi June 25, 2023 11:24

aufi requested changes Jun 26, 2023

View reviewed changes

reviewed code review to check passing CI

477c417

Signed-off-by: Yash Khare <[email protected]>

aufi reviewed Jun 28, 2023

View reviewed changes

added application and dependency check with review

b1fdbc5

Signed-off-by: Yash Khare <[email protected]>

aufi reviewed Jun 28, 2023

View reviewed changes

khareyash05 added 2 commits June 29, 2023 18:50

added download root

2f3835b

Signed-off-by: Yash Khare <[email protected]>

added comparison functionality for both csv files

619f697

Signed-off-by: Yash Khare <[email protected]>

khareyash05 requested a review from aufi June 29, 2023 14:16

remove created file after comparison

af3d42c

Signed-off-by: Yash Khare <[email protected]>

aufi reviewed Jun 29, 2023

View reviewed changes

refactor comparison and add to test-api-matrix

adffc15

Signed-off-by: Yash Khare <[email protected]>

khareyash05 marked this pull request as ready for review June 30, 2023 03:31

khareyash05 requested a review from aufi June 30, 2023 03:31

aufi requested changes Jun 30, 2023

View reviewed changes

khareyash05 added 2 commits July 1, 2023 12:04

added equality checks to the code

9762064

Signed-off-by: Yash Khare <[email protected]>

added valid count check for import

ee0db77

Signed-off-by: Yash Khare <[email protected]>

aufi approved these changes Jul 3, 2023

View reviewed changes

minor changes in the docs

128d807

Signed-off-by: Yash Khare <[email protected]>

aufi merged commit f8f1e0d into konveyor:main Jul 3, 2023

aufi added the test/api Hub API tests-related work label Jul 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 add csv import test #390

🌱 add csv import test #390

khareyash05 commented Jun 13, 2023 •

edited

Loading

aufi left a comment •

edited

Loading

khareyash05 commented Jun 14, 2023

khareyash05 commented Jun 16, 2023

aufi left a comment

aufi Jun 16, 2023

khareyash05 Jun 16, 2023

aufi Jun 16, 2023

djzager Jun 26, 2023

khareyash05 Jun 27, 2023

aufi Jun 16, 2023

aufi left a comment •

edited

Loading

aufi Jun 26, 2023

khareyash05 Jun 26, 2023

aufi Jun 28, 2023

aufi Jun 26, 2023

khareyash05 Jun 26, 2023

aufi Jun 28, 2023

aufi Jun 28, 2023

khareyash05 Jun 28, 2023

aufi Jun 28, 2023

aufi Jun 28, 2023

khareyash05 Jun 28, 2023

aufi Jun 29, 2023

khareyash05 Jun 29, 2023

aufi left a comment

aufi Jun 30, 2023

aufi Jun 30, 2023

aufi Jun 30, 2023

aufi Jun 30, 2023

khareyash05 Jun 30, 2023

aufi Jul 3, 2023

aufi left a comment

aufi Jul 3, 2023

aufi commented Jul 3, 2023

🌱 add csv import test #390

🌱 add csv import test #390

Conversation

khareyash05 commented Jun 13, 2023 • edited Loading

aufi left a comment • edited Loading

Choose a reason for hiding this comment

khareyash05 commented Jun 14, 2023

khareyash05 commented Jun 16, 2023

aufi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aufi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aufi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aufi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aufi commented Jul 3, 2023

khareyash05 commented Jun 13, 2023 •

edited

Loading

aufi left a comment •

edited

Loading

aufi left a comment •

edited

Loading