chore: add intg tests to resource_pool.go #9199

carolinaecalderon · 2024-04-18T22:08:40Z

Ticket

Description

Add tests to resource_pool.go for exported functions on kubernetesResourcePool. These tests run on a minikube cluster that circleCI spins up before running all master intg tests.

Test Plan

See attached test.

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

netlify · 2024-04-18T22:08:58Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`32ea2d8`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6638f8270a71900008731b5f
😎 Deploy Preview	https://deploy-preview-9199--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-04-30T14:38:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.24%. Comparing base (efbcdee) to head (7637063).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9199      +/-   ##
==========================================
+ Coverage   45.14%   45.24%   +0.09%     
==========================================
  Files        1230     1230              
  Lines      154572   154572              
  Branches     2405     2405              
==========================================
+ Hits        69786    69932     +146     
+ Misses      84591    84445     -146     
  Partials      195      195

Flag	Coverage Δ
backend	`42.04% <ø> (+0.28%)`	⬆️
harness	`64.07% <ø> (ø)`
web	`36.06% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 6 files with indirect coverage changes

stoksc

i think this is a fine start but most of the tests are too shallow to be meaningfully effective. e.g., i'd like the max slots test to actually check jobs exceeding max slots get queued, i'd like test allocate/release to actually start the allocation it gets, etc -- and i'd like to have a lot more asserts about the external system (k8s) state while they are ongoing. i approving this (modulo anything i've commented that you can easily fix) and we can try to fill the tests out on top of my pods->jobs pr.

in trying to make these tests more effective, i think it would be useful to run these tests in isolation and look at code coverage to see what paths are getting hit.

master/internal/rm/kubernetesrm/resource_pool_intg_test.go

stoksc · 2024-05-06T16:46:31Z

master/internal/rm/kubernetesrm/resource_pool_intg_test.go

+					Name:         uuid.NewString(),
+					BlockedNodes: []string{uuid.NewString(), uuid.NewString()},
+				}
+				rp.AllocateRequest(allocReq)


i think this test would be a lot more useful if it started the resources it gets and makes some asserts about the state of the kubernetes cluster. but i can add that onto my pr since im changing some of this fundementally.

master/internal/rm/kubernetesrm/resource_pool_intg_test.go

.circleci/real_config.yml

master/internal/rm/kubernetesrm/resource_pool_intg_test.go

cla-bot · 2024-05-09T20:32:50Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @determined-ci, @djanicekpach, @ShreyaLnuHpe on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla.

After we approve your CLA, we will update the contributors list (private) and comment @cla-bot[bot] check to rerun the check.

determined-ci · 2024-05-09T20:32:59Z

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

cla-bot · 2024-05-09T21:49:45Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @determined-ci, @djanicekpach, @ShreyaLnuHpe on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla.

After we approve your CLA, we will update the contributors list (private) and comment @cla-bot[bot] check to rerun the check.

determined-ci · 2024-05-09T21:49:56Z

Docsite preview being generated for this PR.
You can (eventually) find the generated docsite here.

dannysauer

LGTM. Bummer the orb didn't work out as planned.

This reverts commit 54fb10a.

cla-bot bot added the cla-signed label Apr 18, 2024

carolinaecalderon force-pushed the carolinac/rm-201 branch from c78830d to 0abbfd4 Compare April 30, 2024 14:12

carolinaecalderon marked this pull request as ready for review April 30, 2024 16:38

carolinaecalderon requested review from a team as code owners April 30, 2024 16:38

carolinaecalderon requested review from NicholasBlaskey, dannysauer and stoksc April 30, 2024 16:38

carolinaecalderon force-pushed the carolinac/rm-201 branch from e243aed to 32ea2d8 Compare May 6, 2024 15:32

carolinaecalderon changed the base branch from main to stoksc/feat/pods2jobs May 6, 2024 15:35

carolinaecalderon requested a review from a team as a code owner May 6, 2024 15:35

carolinaecalderon changed the base branch from stoksc/feat/pods2jobs to stoksc/feat/kubernetesjobs May 6, 2024 15:35

stoksc approved these changes May 6, 2024

View reviewed changes

dannysauer reviewed May 7, 2024

View reviewed changes

.circleci/real_config.yml Show resolved Hide resolved

carolinaecalderon force-pushed the carolinac/rm-201 branch from 04383f7 to e9d34b4 Compare May 7, 2024 20:53

stoksc reviewed May 8, 2024

View reviewed changes

master/internal/rm/kubernetesrm/resource_pool_intg_test.go Outdated Show resolved Hide resolved

carolinaecalderon force-pushed the carolinac/rm-201 branch from b409ee9 to 1c1f9be Compare May 9, 2024 20:32

carolinaecalderon requested review from a team as code owners May 9, 2024 20:32

carolinaecalderon requested review from EmilyBonar and jgongd and removed request for a team May 9, 2024 20:32

cla-bot bot removed the cla-signed label May 9, 2024

determined-ci requested a review from a team May 9, 2024 20:33

determined-ci added the documentation Improvements or additions to documentation label May 9, 2024

determined-ci removed the documentation Improvements or additions to documentation label May 9, 2024

carolinaecalderon removed request for NicholasBlaskey, a team, EmilyBonar and jgongd May 9, 2024 20:34

carolinaecalderon force-pushed the carolinac/rm-201 branch from 34d42b7 to 97efc29 Compare May 9, 2024 21:36

carolinaecalderon added 10 commits May 9, 2024 22:49

chore: add intg tests to resource_pool.go

459a21a

replacing with orb

47d887d

install minikube

bb53efc

edits

8e8d78b

testing this?

f59df58

undoing orb changes

ae6ec66

todo; fix race conditions

9df194c

edits

5b2a21d

removing recoverJob test

7cb4071

final edits

7637063

carolinaecalderon force-pushed the carolinac/rm-201 branch from 94aff2a to 7637063 Compare May 9, 2024 21:49

cla-bot bot removed the cla-signed label May 9, 2024

determined-ci requested a review from a team May 9, 2024 21:49

determined-ci added the documentation Improvements or additions to documentation label May 9, 2024

carolinaecalderon changed the base branch from stoksc/feat/kubernetesjobs to main May 9, 2024 21:50

carolinaecalderon removed the request for review from a team May 9, 2024 21:52

dannysauer approved these changes May 9, 2024

View reviewed changes

carolinaecalderon merged commit 54fb10a into main May 13, 2024
84 of 98 checks passed

carolinaecalderon deleted the carolinac/rm-201 branch May 13, 2024 14:21

carolinaecalderon added a commit that referenced this pull request May 13, 2024

Revert "chore: add intg tests to resource_pool.go (#9199)"

798e2a2

This reverts commit 54fb10a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add intg tests to resource_pool.go #9199

chore: add intg tests to resource_pool.go #9199

carolinaecalderon commented Apr 18, 2024 •

edited

Loading

netlify bot commented Apr 18, 2024 •

edited

Loading

codecov bot commented Apr 30, 2024 •

edited

Loading

stoksc left a comment

stoksc May 6, 2024

cla-bot bot commented May 9, 2024

determined-ci commented May 9, 2024

cla-bot bot commented May 9, 2024

determined-ci commented May 9, 2024

dannysauer left a comment

chore: add intg tests to resource_pool.go #9199

chore: add intg tests to resource_pool.go #9199

Conversation

carolinaecalderon commented Apr 18, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Apr 18, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented Apr 30, 2024 • edited Loading

Codecov Report

stoksc left a comment

Choose a reason for hiding this comment

stoksc May 6, 2024

Choose a reason for hiding this comment

cla-bot bot commented May 9, 2024

determined-ci commented May 9, 2024

cla-bot bot commented May 9, 2024

determined-ci commented May 9, 2024

dannysauer left a comment

Choose a reason for hiding this comment

carolinaecalderon commented Apr 18, 2024 •

edited

Loading

netlify bot commented Apr 18, 2024 •

edited

Loading

codecov bot commented Apr 30, 2024 •

edited

Loading