Skip to content

refactor(test): refactor e2e test #252

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
Closed
wants to merge 5 commits into from
Closed

refactor(test): refactor e2e test #252

wants to merge 5 commits into from

Conversation

@jimexist
Copy link
Member

@jimexist jimexist commented Dec 28, 2017 •
edited by jlewi
Loading

refactor the test to break down into smaller functions for better readability

This change is Reviewable

@jimexist jimexist changed the title feat(test): refactor e2e test refactor(test): refactor e2e test Dec 28, 2017
@k8s-ci-robot
Copy link

k8s-ci-robot commented Dec 28, 2017

Hi @jimexist. Thanks for your PR.

I'm waiting for a kubernetes or tensorflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@coveralls
Copy link

coveralls commented Dec 28, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 37.915% when pulling b73eb1a on Jimexist:refactor-e2e into 420f9aa on tensorflow:master.

zjj2wry
zjj2wry reviewed Dec 28, 2017
View reviewed changes
test/e2e/main.go
k8s_errors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/api/core/v1"
k8sErrors "k8s.io/apimachinery/pkg/api/errors"
Copy link
Member

@zjj2wry zjj2wry Dec 28, 2017

Choose a reason for hiding this comment

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

apierrors

zjj2wry
zjj2wry reviewed Dec 28, 2017
View reviewed changes
test/e2e/main.go

if !isTBDeployDeleted {
err = fmt.Errorf("TensorBoard deployment %v was not successfully deleted for TfJob %v.", tbDeployName, name)
return
Copy link
Member

@zjj2wry zjj2wry Dec 28, 2017

Choose a reason for hiding this comment

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

remove this return

Copy link
Member Author

@jimexist jimexist Jan 10, 2018

Choose a reason for hiding this comment

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

this is early return, prefer to keep it future-proof

zjj2wry
zjj2wry reviewed Dec 28, 2017
View reviewed changes
test/e2e/main.go
jobName := fmt.Sprintf("%v-%v-%v-%v", fmt.Sprintf("%.40s", original.Metadata.Name), baseName, created.Spec.RuntimeId, i)

_, err := kubeCli.BatchV1().Jobs(Namespace).Get(jobName, metav1.GetOptions{})

Copy link
Member

@zjj2wry zjj2wry Dec 28, 2017

Choose a reason for hiding this comment

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

remove this null line

zjj2wry
zjj2wry reviewed Dec 28, 2017
View reviewed changes
test/e2e/main.go Outdated
for _, r := range original.Spec.ReplicaSpecs {
baseName := strings.ToLower(string(r.TfReplicaType))

for i := 0; i < int(*r.Replicas); i += 1 {
Copy link
Member

@zjj2wry zjj2wry Dec 28, 2017

Choose a reason for hiding this comment

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

i++?

@zjj2wry
Copy link
Member

zjj2wry commented Dec 28, 2017 •
edited
Loading

i did not read this code logic, most lgtm, let it go test~

@zjj2wry
Copy link
Member

zjj2wry commented Dec 28, 2017

/ok-to-test

@jimexist
Copy link
Member Author

jimexist commented Jan 9, 2018

@jlewi is this valuable? if not i'm going to close this PR

@jlewi
Copy link
Contributor

jlewi commented Jan 9, 2018

Yes I think its useful. I was waiting for you to address @zjj2wry 's comments. As soon as those are addressed and merge conflicts are resolved its good to go.

@coveralls
Copy link

coveralls commented Jan 10, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 30.446% when pulling b3ab9ee on Jimexist:refactor-e2e into 9c93d5f on tensorflow:master.

@coveralls
Copy link

coveralls commented Jan 10, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.1%) to 30.551% when pulling b3ab9ee on Jimexist:refactor-e2e into 9c93d5f on tensorflow:master.

@jimexist
Copy link
Member Author

jimexist commented Jan 10, 2018

@zjj2wry @jlewi comment addressed

@coveralls
Copy link

coveralls commented Jan 11, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 30.624% when pulling 502b619 on Jimexist:refactor-e2e into 7ddafd5 on tensorflow:master.

@jlewi
Copy link
Contributor

jlewi commented Jan 12, 2018

@jimexist Thanks. I'm waiting until we get head fixed and can trust the tests again before merging this.

@jlewi
Copy link
Contributor

jlewi commented Jan 16, 2018

@jimexist Head is now fixed. Can you sync and push please so we rerun the tests?

@jimexist
Copy link
Member Author

jimexist commented Jan 19, 2018

@DjangoPeng @jlewi PTAL

@coveralls
Copy link

coveralls commented Jan 19, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 31.439% when pulling bec9b03 on Jimexist:refactor-e2e into 04425d9 on tensorflow:master.

@k8s-ci-robot
Copy link

k8s-ci-robot commented Jan 19, 2018

@jimexist: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
tf-k8s-presubmit bec9b03 link /test tf-k8s-presubmit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@jimexist
Copy link
Member Author

jimexist commented Jan 19, 2018

can't interpret the error messages. seems like 6 out of 6 tests passed but then it complaint about missing junit xml files.

@jlewi
Copy link
Contributor

jlewi commented Jan 20, 2018

@jimexist The missing junit_xml files means some of the tests didn't run which is why its marked as a failure.

@jimexist
Copy link
Member Author

jimexist commented Jan 31, 2018

too many conflicts, abort.

@jimexist jimexist closed this Jan 31, 2018
@jimexist jimexist deleted the refactor-e2e branch January 31, 2018 06:44
sutaakar pushed a commit to sutaakar/training-operator that referenced this pull request Mar 25, 2025
@konflux-internal-p02
Update Konflux references (kubeflow#252)
a8b40a4 
Signed-off-by: konflux-internal-p02 <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
Co-authored-by: konflux-internal-p02[bot] <170854209+konflux-internal-p02[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@zjj2wry zjj2wry zjj2wry left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jimexist @k8s-ci-robot @coveralls @zjj2wry @jlewi