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

Test editing resources in Managed and Unmanaged console-operator state #117

Merged
merged 2 commits into from
Feb 1, 2019
Merged

Test editing resources in Managed and Unmanaged console-operator state #117

merged 2 commits into from
Feb 1, 2019

Conversation

jhadvig
Copy link
Member

@jhadvig jhadvig commented Jan 29, 2019

@benjaminapetersen adding promised tests.
Per our discussion, the tests are editing the resources separately, not bulk, since they have different semantics for the test.
The tests are testing ConfigMap, Service and Route. I've skipped the Deployment since the console operator is not managing changes user makes to it(which is kinda weird?).

Also made the getResource() public -> GetResource(), so it can be used on different places + now it's returning runtime.Object so different fields object field can be accessed.

PTAL

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 29, 2019
@benjaminapetersen
Copy link
Contributor

legitimate test errors i think

benjaminapetersen
benjaminapetersen reviewed Jan 30, 2019
View reviewed changes
pkg/testframework/framework.go
@@ -39,8 +44,8 @@ func DeleteAll(t *testing.T, client *Clientset) {
}
}

func getResource(client *Clientset, resource string) (metav1.Object, error) {
var res metav1.Object
func GetResource(client *Clientset, resource string) (runtime.Object, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

runtime.Object too generic? I think we lose type safety if we use these. Necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah its too generic and we dont need all of the interface features. Was speaking with @mfojtik and he advised me to use runtime.Object, since its more generic and there is a smaller chance you get an object that we cant handle

benjaminapetersen
benjaminapetersen reviewed Jan 30, 2019
View reviewed changes
pkg/testframework/framework.go
@@ -76,7 +81,7 @@ func deleteResource(client *Clientset, resource string) error {

// DeleteCompletely sends a delete request and waits until the resource and
// its dependents are deleted.
func DeleteCompletely(getObject func() (metav1.Object, error), deleteObject func(*metav1.DeleteOptions) error) error {
func DeleteCompletely(getObject func() (runtime.Object, error), deleteObject func(*metav1.DeleteOptions) error) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

runtime.Object same as above, too generic?

Copy link
Member Author

Choose a reason for hiding this comment

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

As commented above :)

benjaminapetersen
benjaminapetersen reviewed Jan 30, 2019
View reviewed changes
pkg/testframework/framework.go Outdated
})
}

func getUID(obj runtime.Object) types.UID {
configMap, ok := obj.(*corev1.ConfigMap)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if passing a runtime.Object then casting it back is the best approach?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, just found out that the idiomatic way is to pass the runtime.Object into meta.Accessor which will return meta.Object

Copy link
Member Author

Choose a reason for hiding this comment

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

Using meta.Accessor is also more safe cause we can get error if passed object is not supported.

benjaminapetersen
benjaminapetersen reviewed Jan 30, 2019
View reviewed changes
pkg/testframework/framework.go Outdated
@@ -134,12 +156,31 @@ func IsResourceAvailable(errChan chan error, client *Clientset, resource string)
errChan <- err
}

func IsResourceAvailable_(client *Clientset, resource string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another way to do the same thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah... sorry for that ... forgot to clean this :D

benjaminapetersen
benjaminapetersen reviewed Jan 30, 2019
View reviewed changes
test/e2e/util.go Outdated
func patchAndCheckConfigMap(t *testing.T, client *testframework.Clientset) bool {
res, err := testframework.GetResource(client, "ConfigMap")
errorCheck(t, err)
configMap, ok := res.(*corev1.ConfigMap)
Copy link
Contributor

Choose a reason for hiding this comment

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

The casting again concerns me a bit, I see it in all of these functions. Wondering if its a signal the generalized GetResource ought to be a specific function for each resource type w/a specific return type?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah was thinking about it and thought it would be better to have an single get function for all the types then multiple get functions for each type that would also cast object directly. Wasnt really sure which one is more idiomatic.

benjaminapetersen
benjaminapetersen reviewed Jan 30, 2019
View reviewed changes
test/e2e/util.go Outdated
return reflect.DeepEqual(originalData, newData)
}

func errorCheck(t *testing.T, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I sympathize with the helper to get rid of the if block :)

So I think the recommended path is to always return the error, and handle them only at the top in one place.

benjaminapetersen
benjaminapetersen reviewed Jan 30, 2019
View reviewed changes
Copy link
Contributor

@benjaminapetersen benjaminapetersen left a comment

Choose a reason for hiding this comment

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

Quick review, mostly the casting bits make me pause. Yay progress!

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2019
@jhadvig
Copy link
Member Author

jhadvig commented Jan 30, 2019

@benjaminapetersen updated per our conversation

@jhadvig
Copy link
Member Author

jhadvig commented Jan 30, 2019

/retest

@benjaminapetersen
Copy link
Contributor 

clusterversion.config.openshift.io/version condition met
hack/test-e2e.sh
[INFO] Running e2e tests
test/e2e/util.go:10:2: cannot find package "k8s.io/kubernetes/pkg/util/wait" in any of:
	/go/src/github.com/openshift/console-operator/vendor/k8s.io/kubernetes/pkg/util/wait (vendor tree)
	/usr/local/go/src/k8s.io/kubernetes/pkg/util/wait (from $GOROOT)
	/go/src/k8s.io/kubernetes/pkg/util/wait (from $GOPATH)

that one legitimate?

@jhadvig jhadvig force-pushed the edit-resources branch 3 times, most recently from 3628ba4 to da73385 Compare January 31, 2019 13:38
@benjaminapetersen
Copy link
Contributor

ok! lets give these a spin.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, jhadvig

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2019
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 3070353 into openshift:master Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjaminapetersen benjaminapetersen benjaminapetersen left review comments

@spadgett spadgett Awaiting requested review from spadgett

Assignees

@benjaminapetersen benjaminapetersen

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jhadvig @benjaminapetersen @openshift-ci-robot @openshift-bot @openshift-merge-robot