🌱 add dependency package in test/api #375

khareyash05 · 2023-06-06T05:47:50Z

Issue in the repo #370

added api_test.go,pkg.go,samples.go in dependency directory. Also added dependency.go in binding package and added dependency client in binding/richclient.go

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

khareyash05 · 2023-06-06T07:17:39Z

Working on handling the Samples array to take TestCase as argument as it expects []api.Dependency{}

aufi

Hey Yash, thanks for your PR 👍 There are few things we need to work on (comments in the source code), but overall looks good!

binding/dependency.go

aufi · 2023-06-06T09:49:58Z

test/api/dependency/api_test.go

+	for _, r := range Samples {
+		t.Run("Dependency_CRUD", func(t *testing.T) {
+			// Create.
+			err := Dependency.Create(&r)


The API Dependency links two Applications and keeps their IDs in From/To fields. These fields are Refs (https://github.com/konveyor/tackle2-hub/blob/main/api/base.go#L379) so contain just IDs (and optionally Name, not that not important), but the Refs are not actual Applications. The From and To Applications need to be created before creating Dependency.

As I understood from the review that dependency need To/From fields which will be used to refer to the applications?

and that To/From will be generated by creating an instance of application in api_test.go. Am i in the right direction?

aufi · 2023-06-06T09:53:07Z

test/api/dependency/api_test.go

+
+	for name := range samples {
+		sample := samples[name]
+		assert.Must(t, Dependency.Create(&sample))


Same with creating Dependencies - 1st Applications, 2nd Dependency between them.

test/api/dependency/samples.go

aufi · 2023-06-06T10:08:35Z

test/api/dependency/samples.go

+	}
+	tc      = []TestCase{testCase}
+	Samples = []api.Dependency{tc}
+)


Definition of Dependencies (and likely also TestCases) will need to be updated. There is some limitation caused by api.Dependency keeping just Ref of Aplication, so we need store application elsewhere. I can see few options:

Use TestCases struct that might even not contain the Dependency itself, but something like just ApplicationFrom and ApplicationTo, so the test will go through testcases and create ApplicationTo, ApplicationFrom and then dependency between these applications (the dependency doesn't have any special fields than the Application refs, so it should work well).

Use model.Dependency model, that stores links to Application (not just Refs), see https://github.com/konveyor/tackle2-hub/blob/main/migration/v2/model/dependency.go#L12-L18 (yes, we do have slightly different models for database and API). The Samples array would be array of these model.Dependency and test could create both Applications and "convert" the model.Dependency to api.Dependency using method With() https://github.com/konveyor/tackle2-hub/blob/main/api/dependency.go#L157-L161

It would great if you can try both ways (or maybe find something else if you'd like it more than suggestions above) and we can discuss how it works if needed.

Definition of Dependencies (and likely also TestCases) will need to be updated. There is some limitation caused by api.Dependency keeping just Ref of Aplication, so we need store application elsewhere. I can see few options:

Use TestCases struct that might even not contain the Dependency itself, but something like just ApplicationFrom and ApplicationTo, so the test will go through testcases and create ApplicationTo, ApplicationFrom and then dependency between these applications (the dependency doesn't have any special fields than the Application refs, so it should work well).

Use model.Dependency model, that stores links to Application (not just Refs), see https://github.com/konveyor/tackle2-hub/blob/main/migration/v2/model/dependency.go#L12-L18 (yes, we do have slightly different models for database and API). The Samples array would be array of these model.Dependency and test could create both Applications and "convert" the model.Dependency to api.Dependency using method With() https://github.com/konveyor/tackle2-hub/blob/main/api/dependency.go#L157-L161

It would great if you can try both ways (or maybe find something else if you'd like it more than suggestions above) and we can discuss how it works if needed.

I think 1st option is easier to implement because it involves less complexities and 2nd option is is less intuitive which can be easily dealt by the 1st. What's your say on that?

I have a thought process of injecting dependency withdependecy-less TestCase in Samples[]. and then use From/To in the api_test.go to access them in the dependency to be used by various CRUD functions.

I'm not sure I fully understand, but sounds good.

A Followup to this discussion and the 1st option, the dependency variable (and Samples containing only the dependency when api_test.go creates new applicationFrom/To instances) here seems to me to be overcomplicated.
What about drop the dependency variable, considering update/rename TestCase and putting to Samples (or renamed array for tests) and doing all this in since var( ... ) block?

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

aufi

Thanks for update! Left few comments, let's sync tomorrow.

test/api/dependency/api_test.go

aufi · 2023-06-06T15:57:19Z

test/api/dependency/samples.go

+}
+
+var (
+	dependency = api.Dependency{


Some of these lines seems to be redundant to me (the dependency here and init() below). But I'd suggest get it to (partially) working state (passing tests against locally running Hub https://github.com/konveyor/tackle2-hub/tree/main/test#rest-api) and we can refactor it then.

Refactored the code Kindly review

test/api/dependency/api_test.go

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

aufi

Thanks for update @khareyash05, I'm happy that there is already a working/test passing code, great job!

I can see two things to be done before mering this PR:

remove unnecessary parts from the code (comments inline)
add test to ensure there are no cyclic dependencies
- direct: when Gateway->Inventory dependency exists, API shoud not allow Inventory->Gateway dependency to be created
- indirect: when Gateway->Inventory and Inventory->Database dependencies exist, API shoud not allow Database->Gateway dependency to be created
  (just for info, related code inside the Hub is https://github.com/konveyor/tackle2-hub/blob/main/migration/v2/model/dependency.go#L30)

api/dependency.go

test/api/dependency/api_test.go

test/api/dependency/pkg.go

aufi · 2023-06-07T14:49:37Z

test/api/dependency/samples.go

+	}
+	tc      = []TestCase{testCase}
+	Samples = []api.Dependency{tc}
+)


A Followup to this discussion and the 1st option, the dependency variable (and Samples containing only the dependency when api_test.go creates new applicationFrom/To instances) here seems to me to be overcomplicated.
What about drop the dependency variable, considering update/rename TestCase and putting to Samples (or renamed array for tests) and doing all this in since var( ... ) block?

aufi · 2023-06-07T14:50:54Z

test/api/dependency/api_test.go

+	for _, r := range Samples {
+		t.Run(fmt.Sprintf("Dependency from %s -> %s", r.From.Name, r.To.Name), func(t *testing.T) {
+
+			applicationFrom := api.Application{


I'm not sure about creating new instances of applicationFrom and To, but that depends on samples.go, see comment there.

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

khareyash05 · 2023-06-08T06:11:38Z

TODO: Cyclic Dependency.

aufi

Looks good! Added few minor comments, once resolved those, I'm OK merging this PR. The cyclic dependency test can be in a follow-up PR.

api/dependency.go

test/api/dependency/api_test.go

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

aufi

👍 Needed some clarification on Must/Should and fix List test - recommend run the test locally more than once to be sure, that one test run doesn't leave data for another test run.

test/api/dependency/api_test.go

aufi · 2023-06-08T10:57:15Z

test/api/dependency/api_test.go

+
+		// Create applications.
+		assert.Should(t, Application.Create(&sample.ApplicationFrom))
+		assert.Should(t, Application.Create(&sample.ApplicationTo))


Lines assigning IDs to sample to allow its deletion in the end of the test should have stay here (sample.ApplicationFrom.ID = applicationFrom.ID).

but we are deleting applicationFrom.ID as it is not needed. I am failing to understand as to why will we need this line

aufi · 2023-06-08T10:58:38Z

test/api/dependency/api_test.go

+		assert.Should(t, Application.Create(&sample.ApplicationFrom))
+		assert.Should(t, Application.Create(&sample.ApplicationTo))
+	}
+


There should be a Dependency creation and condition below should check if got is an array with the created dependency. Sorry, I missed this part before.

Shall I just compare the ID's of dependency created and the ApplicationFrom/To ID's will it suffice?

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

khareyash05 · 2023-06-08T12:58:54Z

@aufi Kindly review

api/dependency.go

aufi · 2023-06-08T14:02:02Z

test/api/dependency/api_test.go

+	}
+	if assert.FlatEqual(got, Samples) {
+		t.Errorf("Different response error. Got %v, expected %v", got, Samples)
+	}


Found out we're using the assert.FlatEqual method in a wrong way (also in some other tests outside of this PR), the condition should look like if !assert.FlatEqual(got, Samples) { //error

Was wondering too when working on the part before! This might be a breaking change in the code, as inverting the condition will make the test fail. Will dig deep on the same .

aufi · 2023-06-08T14:06:22Z

test/api/dependency/api_test.go

+	}
+
+	// Delete Dependencies.
+	for _, dependency := range got {


Thihs would delete all dependencies in API, not only ones created by this test. Not saying it is blocker in this case, but better track created objects to be deleted.

Ya that's something should be taken care of. A way to tackle this what I can think of is creating an array of ID's and store ID's of dependencies at their time of creation. The Delete() of the Dependency will work on the array of ID's and will only delete them.

As we have (or we will) ensure that all the created Dependencies are present in the Dependency.List(), we will ensure that all the dependencies are used and thus they are allowed to be deleted.

test/api/dependency/api_test.go

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

aufi

There are some things (assert with FlatEqual and add cyclic test), that need to be updated in followup PR, but:

Looks good to me, nice work Yash!

khareyash05 · 2023-06-09T07:11:31Z

There are some things (assert with FlatEqual and add cyclic test), that need to be updated in followup PR, but:

Looks good to me, nice work Yash!

Thank you and will do that in the followup PR

add dependency directory necessary files

6ed6cd0

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

khareyash05 force-pushed the test-api-dependency branch from 03b021e to 6ed6cd0 Compare June 6, 2023 05:55

khareyash05 changed the title ~~add dependency directory necessary files~~ :seedling: add dependency directory necessary files Jun 6, 2023

khareyash05 changed the title ~~:seedling: add dependency directory necessary files~~ 🌱 add dependency directory necessary files Jun 6, 2023

khareyash05 changed the title ~~🌱 add dependency directory necessary files~~ 🌱 add dependency package in test/api Jun 6, 2023

aufi self-requested a review June 6, 2023 07:03

updated sample test case but errors still exist

06e448e

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

aufi requested changes Jun 6, 2023

View reviewed changes

updated test case to add application with dependency

a8449a5

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

khareyash05 requested a review from aufi June 6, 2023 13:45

aufi reviewed Jun 6, 2023

View reviewed changes

added application.create()calls and refactor code

a07ee95

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

khareyash05 requested a review from aufi June 7, 2023 03:28

add missing return dependency and delete Applications after use

5ad6f23

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

khareyash05 marked this pull request as ready for review June 7, 2023 12:49

aufi reviewed Jun 7, 2023

View reviewed changes

aufi added the test/api Hub API tests-related work label Jun 7, 2023

refactor code to pass in Applications

3f4253c

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

aufi reviewed Jun 8, 2023

View reviewed changes

api/dependency.go Show resolved Hide resolved

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

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

minor code changes and refactor the code

08ad217

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

khareyash05 requested a review from aufi June 8, 2023 10:25

aufi reviewed Jun 8, 2023

View reviewed changes

khareyash05 added 2 commits June 8, 2023 18:25

updated List to change query paramters

cb13080

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

updated List

890a794

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

khareyash05 requested a review from aufi June 8, 2023 13:00

aufi reviewed Jun 8, 2023

View reviewed changes

api/dependency.go Outdated Show resolved Hide resolved

aufi reviewed Jun 8, 2023

View reviewed changes

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

khareyash05 marked this pull request as draft June 8, 2023 15:42

major dependency checks and reimplementation

16efa1c

Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>

aufi approved these changes Jun 9, 2023

View reviewed changes

aufi marked this pull request as ready for review June 9, 2023 07:03

aufi mentioned this pull request Jun 9, 2023

Add API test for Dependency resource #370

Closed

aufi merged commit 69ca6ac into konveyor:main Jun 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 add dependency package in test/api #375

🌱 add dependency package in test/api #375

khareyash05 commented Jun 6, 2023 •

edited

Loading

khareyash05 commented Jun 6, 2023 •

edited

Loading

aufi left a comment •

edited

Loading

aufi Jun 6, 2023

khareyash05 Jun 6, 2023

khareyash05 Jun 6, 2023

aufi Jun 6, 2023

aufi Jun 6, 2023

aufi Jun 6, 2023 •

edited

Loading

khareyash05 Jun 6, 2023

khareyash05 Jun 6, 2023

aufi Jun 6, 2023

aufi Jun 7, 2023

aufi left a comment

aufi Jun 6, 2023

khareyash05 Jun 7, 2023

aufi left a comment

aufi Jun 7, 2023

aufi Jun 7, 2023

khareyash05 commented Jun 8, 2023

aufi left a comment

aufi left a comment •

edited

Loading

aufi Jun 8, 2023

khareyash05 Jun 8, 2023 •

edited

Loading

aufi Jun 8, 2023

khareyash05 Jun 8, 2023

khareyash05 commented Jun 8, 2023

aufi Jun 8, 2023

khareyash05 Jun 8, 2023 •

edited

Loading

aufi Jun 8, 2023

khareyash05 Jun 8, 2023

khareyash05 Jun 8, 2023

aufi left a comment

khareyash05 commented Jun 9, 2023

🌱 add dependency package in test/api #375

🌱 add dependency package in test/api #375

Conversation

khareyash05 commented Jun 6, 2023 • edited Loading

khareyash05 commented Jun 6, 2023 • edited Loading

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

aufi Jun 6, 2023 • 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

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

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

khareyash05 commented Jun 8, 2023

aufi left a 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

khareyash05 Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khareyash05 commented Jun 8, 2023

Choose a reason for hiding this comment

khareyash05 Jun 8, 2023 • 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

aufi left a comment

Choose a reason for hiding this comment

khareyash05 commented Jun 9, 2023

khareyash05 commented Jun 6, 2023 •

edited

Loading

khareyash05 commented Jun 6, 2023 •

edited

Loading

aufi left a comment •

edited

Loading

aufi Jun 6, 2023 •

edited

Loading

aufi left a comment •

edited

Loading

khareyash05 Jun 8, 2023 •

edited

Loading

khareyash05 Jun 8, 2023 •

edited

Loading