-
Notifications
You must be signed in to change notification settings - Fork 35
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
🌱 add migrationwave API test #471
Conversation
Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>
Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>
Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>
test/api/migrationwave/api_test.go
Outdated
"github.com/konveyor/tackle2-hub/test/assert" | ||
) | ||
|
||
func TestReviewCRUD(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestMigrationWaveCRUD
binding/migrationwave.go
Outdated
} | ||
|
||
// | ||
// List Reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List MigrationWaves.
test/api/migrationwave/api_test.go
Outdated
// Compare contents of migration waves. | ||
for _, createdMigrationWave := range createdMigrationWaves { | ||
found := false | ||
for _, retrievedReview := range got { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
retrievedWave
test/api/migrationwave/api_test.go
Outdated
} | ||
} | ||
if !found { | ||
t.Errorf("Expected review not found in the list: %v", createdMigrationWave) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expected wave
Oh missed that when using the same elements as Reviews. Will Update all. Thank you for the review. |
Related to konveyor/operator#220 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me, added few comments. Thanks!
Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>
Name: stakeholder.Name, | ||
Email: "sample@example.com", | ||
} | ||
assert.Must(t, Stakeholder.Create(&expectedStakeholder)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one thing I missed before. I understand you cannot use r.Applications or r.Stakeholders directly since they're just Refs, but you need to preserve Refs IDs.
Currently this test creates Application&Stakeholder&StakeholderGroup with ID 1 (on empty database) and tries delete Application ID 1, Stakeholder ID 2, StakeholderGroup ID 3 (and leaves created Stakeholder&StakehoderGroup in the database).
Use IDs from Refs when creating these resources and ensure (just check manually on your local dev env) there are no remains in database after test execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya that seems an issue.Noticed how. Thanks for the check will update them
And update API test matrix (as in previous PRs).. |
Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>
Thanks for update, but I'm afraid I'm not OK with the latest change. Plus one on passing test and not leaving orphaned database records 👍 But not creating dependencies before creating migration wave doesn't looks right to me. The side-effect that the Create action on Migration wave creates dependencies from its Refs is something I don't see as correct (not saying it is a bug, just an edge case), not sure how my colleagues feel about this, but for my approval, please create (and delete) dependent objects for the migration wave manually. (that should match to typical use-case in UI where user creates migration wave first and then adds applications to the migration wave) |
test/api/migrationwave/api_test.go
Outdated
@@ -11,29 +10,6 @@ import ( | |||
|
|||
func TestMigrationWaveCRUD(t *testing.T) { | |||
for _, r := range Samples { | |||
for _, app := range r.Applications { | |||
expectedApp := api.Application{ | |||
Name: app.Name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add ID here before the Name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid it doesnt have Id in struct rather I added in the samples.go
have a look?
Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a small problem with creating dependencies in API call together with the MigratioWave, but since API allows it, not a blocker. LGTM.
Let's discuss this . Might have missed something. |
Added the migration wave API test