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

add unit tests for tensorflow controller #1511

Merged
merged 3 commits into from
Jan 6, 2022
Merged

add unit tests for tensorflow controller #1511

merged 3 commits into from
Jan 6, 2022

Conversation

zw0610
Copy link
Member

@zw0610 zw0610 commented Dec 18, 2021 •
edited

What this PR does / why we need it:
This pr convert the unit test for previous tf-operator (tag: v1.1.0) to tf-controller in training-operator.

Which issue(s) this PR fixes (optional, in Fixes #<issue number>, #<issue number>, ... format, will close the issue(s) when PR gets merged):
Fixes #1497

Checklist:

controller_test.go

  • TestNormalPath
  • TestRun

job_test.go

  • TestAddTFJob
  • TestCopyLabelsAndAnnotation
  • TestDeletePodsAndServices
    - [ ] TestCleanupTFJob (removed in v1.1.0)
  • TestActiveDeadlineSeconds
  • TestBackoffForOnFailure

pod_test.go

  • TestAddPod
  • TestExpectation (may not fit in reconciler/controller mode)
  • TestExpectationWithError (may not fit in reconciler/controller mode)
  • TestClusterSpec
  • TestIsDistributed
  • TestRestartPolicy
  • TestExitCode
  • TestScaleDown
  • TestScaleUp
  • TestIsWorker0Completed

status_test.go

  • TestFailed
  • TestStatus

tensorflow_test.go

  • TestConvertClusterSpecToSparseClusterSpec

util_test.go

  • TestGenOwnerReference
  • TestGenLabels
    - [ ] TestConvertTFJobToUnstructured (no longer appliable)

@coveralls
Copy link

coveralls commented Dec 18, 2021 •
edited

Pull Request Test Coverage Report for Build 1658954206

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+9.0%) to 36.875%
Files with Coverage Reduction New Missed Lines %
pkg/controller.v1/mpi/mpijob_controller.go 2 75.35%
Totals Coverage Status
Change from base Build 1582221444: 9.0%
Covered Lines: 2263
Relevant Lines: 6137
💛 - Coveralls

@google-oss-prow google-oss-prow bot added size/L and removed size/M labels Dec 18, 2021
@terrytangyuan terrytangyuan marked this pull request as draft December 18, 2021 18:47
@terrytangyuan
Copy link
Member

FYI you can start the PR as draft pull request so others won't get notifications every time you push a new commit. See https://github.blog/2019-02-14-introducing-draft-pull-requests/.

@zw0610 zw0610 force-pushed the ut-tf branch 3 times, most recently from f50d1a0 to f749e74 Compare December 21, 2021 07:15
@zw0610
add unit test for tf
4571b22 
add amend function

add test exit code

add scale up and down cases

add norm test

fix test exit code case

fix import order

fix exit code issue
@zw0610 zw0610 marked this pull request as ready for review December 21, 2021 08:18
@zw0610 zw0610 changed the title WIP: add unit tests for tensorflow controller add unit tests for tensorflow controller Dec 21, 2021
@Jeffwan
Copy link
Member

Jeffwan commented Dec 21, 2021

This is amazing! Thanks for driving the efforts!

Jeffwan
Jeffwan reviewed Dec 21, 2021
View reviewed changes
pkg/common/util/v1/testutil/pod.go Outdated Show resolved Hide resolved
pkg/common/util/v1/testutil/pod.go Show resolved Hide resolved
pkg/common/util/v1/testutil/pod.go

v1 "k8s.io/api/core/v1"
commonv1 "github.com/kubeflow/common/pkg/apis/common/v1"
. "github.com/onsi/gomega"
Copy link
Member

Choose a reason for hiding this comment

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

any reason to import gomega this way?

Copy link
Member Author

Choose a reason for hiding this comment

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

to be consistent with the tutorial of kubebuilder: https://book.kubebuilder.io/cronjob-tutorial/writing-tests.html

pkg/common/util/v1/testutil/service.go Outdated Show resolved Hide resolved
pkg/controller.v1/tensorflow/tfjob_controller_test.go Outdated Show resolved Hide resolved
pkg/controller.v1/tensorflow/suite_test.go Show resolved Hide resolved
pkg/controller.v1/tensorflow/status_test.go Show resolved Hide resolved
@zw0610
fix comment
f7acfc6 
fix test exit code issue

fix pod spec
terrytangyuan
terrytangyuan approved these changes Dec 23, 2021
View reviewed changes
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

This is great to have! Thanks!

@zw0610
Copy link
Member Author

zw0610 commented Dec 27, 2021

@Jeffwan Please take a look at the updated commit.

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jeffwan, terrytangyuan, zw0610

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:
  • OWNERS [Jeffwan,terrytangyuan,zw0610]

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

@Jeffwan
Copy link
Member

Jeffwan commented Jan 6, 2022

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jan 6, 2022
@google-oss-prow google-oss-prow bot merged commit 487f961 into kubeflow:master Jan 6, 2022
@zw0610 zw0610 deleted the ut-tf branch June 20, 2022 04:42
@zw0610 zw0610 restored the ut-tf branch June 20, 2022 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jeffwan Jeffwan Jeffwan approved these changes

@terrytangyuan terrytangyuan terrytangyuan approved these changes

@ChanYiLin ChanYiLin Awaiting requested review from ChanYiLin

@gaocegege gaocegege Awaiting requested review from gaocegege

@jinchihe jinchihe Awaiting requested review from jinchihe

Assignees

@Jeffwan Jeffwan

Labels
approved lgtm size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve test coverage
5 participants
@zw0610 @coveralls @terrytangyuan @Jeffwan @gaocegege