Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Replacing ginkgo test with go testing #248

Closed
wants to merge 8 commits into from
Closed

Replacing ginkgo test with go testing #248

wants to merge 8 commits into from

Conversation

jubittajohn
Copy link
Contributor

Description

Changes for #191

m1kola
m1kola requested changes Jun 2, 2023
View reviewed changes
Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to tidy up a bit, but it is a good start!

internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
m1kola
m1kola reviewed Jun 5, 2023
View reviewed changes
Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to properly review updated version - so far just a small suggestion so far.

I will take a good look tomorrow.

internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
@ankitathomas ankitathomas requested a review from m1kola June 5, 2023 19:02
ankitathomas
ankitathomas reviewed Jun 5, 2023
View reviewed changes
Copy link
Contributor

@ankitathomas ankitathomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few small suggestions, but great work so far!

internal/resolution/resolver_test.go Show resolved Hide resolved
internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
internal/resolution/resolver_test.go Show resolved Hide resolved
ankitathomas
ankitathomas approved these changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@ankitathomas ankitathomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2023
m1kola
m1kola approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only managed to come up with a few minor suggestions, but nothing blocking.

internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
anik120
anik120 requested changes Jun 6, 2023
View reviewed changes
internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
internal/resolution/resolver_test.go Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 6, 2023
m1kola
m1kola approved these changes Jun 7, 2023
View reviewed changes
Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

anik120
anik120 approved these changes Jun 8, 2023
View reviewed changes
Copy link
Contributor

@anik120 anik120 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2023
@jubittajohn
Replacing ginkgo test with go testing
6353e18 
Signed-off-by: jubittajohn <jujohn@redhat.com>
@jubittajohn
Fix formatting
a74f229 
Signed-off-by: jubittajohn <jujohn@redhat.com>
@jubittajohn
Modify expected solution to utilize arguments
e6dc77b 
Signed-off-by: jubittajohn <jujohn@redhat.com>
@jubittajohn
Replace predefined selected variable count with len function
0e59413 
Signed-off-by: jubittajohn <jujohn@redhat.com>
@jubittajohn
Change error condition check to use expected error
ad62fe9 
Signed-off-by: jubittajohn <jujohn@redhat.com>
@jubittajohn
Replace the actual production image with a sample image path and appl…
fe19ce1 
…y minor changes

Signed-off-by: jubittajohn <jujohn@redhat.com>
@jubittajohn
Remove olm.gvk property
a496d68 
Signed-off-by: jubittajohn <jujohn@redhat.com>
@jubittajohn
Change test file to make OLMVariableSource responsible for fetching
1337fca 
Signed-off-by: jubittajohn <jujohn@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jun 8, 2023

New changes are detected. LGTM label has been removed.

@jubittajohn
Copy link
Contributor Author

The resolver.go is removed from the resolution package as a part of making the OLMVariableSource responsible for fetching. As Deppy solver and  OLMVariableSource has own tests, resolver_test.go is no longer required. So this change is not required. Hence closing the PR.

@m1kola m1kola mentioned this pull request Jun 29, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m1kola m1kola m1kola approved these changes

@anik120 anik120 anik120 approved these changes

@ankitathomas ankitathomas Awaiting requested review from ankitathomas

Assignees

@anik120 anik120

@ankitathomas ankitathomas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jubittajohn @m1kola @anik120 @ankitathomas