Adapt tests to reuse Orchestrator #567
Conversation
MattToast
changed the title
MattToast
requested review from
ankona,
amandarichardsonn and
AlyssaCote
and removed request for
ankona
tests/test_fixtures.py
Outdated
| def test_single_db_fixture(single_db): | ||
| experiment = Experiment("test_single_fixture") | ||
| experiment.reconnect_orchestrator(single_db.checkpoint_file) |
There was a problem hiding this comment.
Incomplete test? And if we're testing one of the fixtures, we might want to test all of them!
There was a problem hiding this comment.
Uhhh... good question! I've never written testing code for testing code before. I'll defer to @ashao on this one for intention.
There was a problem hiding this comment.
How meta do we want to go? But yes the intention of this was to make sure that fixture works as expected. I'm imagining us bashing our head against something related to the fixture and just being able to do:
pytest test_fixtures.py::test_single_db_fixture
and save us some time down the line. Happy to be argued down, but in general I think it's not crazy to have a some sanity checking of testing code since it helps to debug tests
There was a problem hiding this comment.
I definitely don't hate the idea of being able to sanity check that the fixtures are working as expected.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #567 +/- ##
===========================================
- Coverage 84.34% 78.71% -5.63%
===========================================
Files 67 67
Lines 4637 4642 +5
===========================================
- Hits 3911 3654 -257
- Misses 726 988 +262
|
| @@ -102,7 +102,7 @@ Detailed Notes | |||
| Torch will unconditionally try to link in this library, however | |||
| fails because the linking flags are incorrect. | |||
| ([SmartSim-PR538](https://github.com/CrayLabs/SmartSim/pull/538)) | |||
| - Change type_extension and pydantic versions in readthedocs | |||
| - Change typing\_extensions and pydantic versions in readthedocs | |||
There was a problem hiding this comment.
I notice two updates to the Detailed Notes section but one update to the Description!
There was a problem hiding this comment.
Yeah the change to "Description" and one of the "Detailed Notes" edits were just minor typo/grammar things.
The actual "Detailed Note" I added I figured fell under the category of "Minor enhancements to test suite" which is already listed in the "Description" section, so I just added something the details. LMK if you think this warrants its on entry in the "Description" and I can add something there!!
|
Note that Al noted a defect which causes cascading failures if the database was stopped on purpose or because of a database error. A fix is coming in |
ashao
force-pushed
the
orchestrator_fixture
branch
5 times, most recently
from
a165c25 to
09aa6f3
Compare
…atch test description
Remove the `is_active` check in the controller as it was leading to node promotion errors within `redispy` when checking the status of a batch launched orchestrator.
ashao
force-pushed
the
orchestrator_fixture
branch
from
3640355 to
3694737
Compare
Tests which needed to launch an Orchestrator were spinning up and shutting down their own instances. This led to a number of cases where a single test failing would cascade into failures of other tests. Additionally, this also meant that a significant amount of time in the tests was spent waiting for Orchestrators to launch.
This PR adds a session-scoped fixture that returns an Orchestrator. Most tests which use an Orchestrator have been updated to use this fixture; the remaining for various reasons still need to spin up their own (for example the multiple database tests need to have a named Orchestrator).
This PR is not 100% complete with the following to be done