test: refactor usage within test_local #8913
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
wes-turner
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8913 +/- ##
=======================================
Coverage 47.22% 47.22%
=======================================
Files 1162 1162
Lines 175910 175910
Branches 2235 2235
=======================================
Hits 83066 83066
Misses 92686 92686
Partials 158 158
Flags with carried forward coverage won't be shown. Click here to find out more. |
wes-turner
force-pushed
the
wes/refactor-test-deploy-local
branch
2 times, most recently
from
221f3aa
to
deed490
Compare
e2e_tests/tests/api_utils.py
Outdated
| @functools.lru_cache(maxsize=1) | ||
| def _get_scheduler_type() -> Optional[bindings.v1SchedulerType]: | ||
| scheduler_type: Optional[bindings.v1SchedulerType] | ||
| @functools.lru_cache() |
There was a problem hiding this comment.
It feels slightly better from an interface perspective to put the optional session parameter (and its creation) into the callers of these primitives (like skipif_not_hpc).
Let's take _get_scheduler_type and skipif_not_hpc explicitly.
If we move the session into skipif_not_hpc, then in the case no session is passed (like during all the skipifs) then a session will be created inside skipif_not_hpc. This session creation involves a login, which may fail after trying to find master if master isn't responsive. So we put that into a try and probably cache skipif_not_hpc, too, so we don't mindlessly try to log in a thousand times.
But the functools cache treats each parameter a function gets as necessary, and skipif_not_hpc also takes a reason. It seems wrong to bust the cache if that reason changes.
I think the design that makes the most sense is for skipif_not_hpc to take a required session. But then this makes pytest have to create those sessions explicitly while it's gathering tests. Which, it's at runtime that we'll be most certain of where master has been brought up, so this makes sense. But it's also a bigger refactor than I want to do, here.
In the end, my preference is for this suboptimal solution rather than the solution that moves session creation to callers.
But maybe that's wrong or maybe there's another approach entirely. Thoughts?
There was a problem hiding this comment.
OOB discussion led to not changing these methods to take an optional session.
| 1 | ||
| if t.trial.state | ||
| in [ | ||
| bindings.trialv1State.RUNNING, | ||
| bindings.trialv1State.STARTING, | ||
| bindings.trialv1State.PULLING, | ||
| ] | ||
| else 0 |
There was a problem hiding this comment.
recommend deleting 1 if and else 0. Just a nit.
There was a problem hiding this comment.
Nice. This let me remove the parens without my formatter adding them back in.
| max_secs_to_free_slots = 300 if api_utils.is_hpc() else 30 | ||
| max_secs_to_free_slots = 300 if api_utils.is_hpc(sess) else 30 |
There was a problem hiding this comment.
is this the only place you have caching issues? Or did I miss another place while skimming through the changes?
There was a problem hiding this comment.
Resolved OOB, this plus one other place I also missed and am adding.
wes-turner
force-pushed
the
wes/refactor-test-deploy-local
branch
from
7b3c07a
to
dbb0a80
Compare
This still allows the skipif decorators to all be cached during test collection, while giving tests themselves the opportunity to pass explicit sessions and get new results from there.
wes-turner
force-pushed
the
wes/refactor-test-deploy-local
branch
from
dbb0a80
to
45b137c
Compare
Fixes `test-det-deploy-local` tests that were broken by a change in caching strategy in #8872. Changes to test_local.py: * changed interface to functions bringing up and down resources to be both more general and more explicit * resources in the tests themselves are brought up and down using a context manager. This ensures they clean up after themselves even when they fail. * deleted a couple functions that had been needed for the previously-deleted `test-stress`. Changes to api_utils.py: * `is_hpc` operates directly on a passed session. Changes to experiment.py & compute-stats.py * The session for a run experiment is passed to `api_utils.is_hpc` so that this function can query the now-live master to determine whether that master is running an HPC scheduler. Previous calls to `is_hpc` only accidentally worked because the very first time it was called it brought up a master that happened to run at the same default location as the session `is_hpc` generated by default. * Formatting
Fixes
test-det-deploy-localtests that were broken by a change in caching strategy in #8872.Changes to test_local.py:
test-stress.Changes to api_utils.py:
is_hpcoperates directly on a passed session.Changes to experiment.py & compute-stats.py
api_utils.is_hpcso that this function can query the now-live master to determine whether that master is running an HPC scheduler. Previous calls tois_hpconly accidentally worked because the very first time it was called it brought up a master that happened to run at the same default location as the sessionis_hpcgenerated by default.Description
Test Plan
Commentary (optional)
Checklist
docs/release-notes/.See Release Note for details.
Ticket