[MNT] Intermediate test plan #1629
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1629 +/- ##
==========================================
+ Coverage 52.04% 52.75% +0.71%
==========================================
Files 36 36
Lines 4333 4333
==========================================
+ Hits 2255 2286 +31
+ Misses 2078 2047 -31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Docker fails to build for Windows, given that the image of database are built using Linux binaries. It works locally because Docker Desktop ends up using WSL in the background (as far as I can understand) to run these containers without errors. Currently I am skipping running docker for the windows env during CI runs only, and using this for tests that will end up using the docker servers: is_windows_ci = (sys.platform == "win32" or os.name == "nt") and os.environ.get("CI") == "true"
skip_on_windows_ci = pytest.mark.skipif(
is_windows_ci,
reason="Skipping Docker tests on Windows CI (No WSL2 available)"
)
# Used in the form:
@skip_on_windows_ci
def test_database_connection():
...Since we eventually plan on migrating all tests to these local servers, and we would like windows to run them too, we should think of another method to run these tests on windows CI. |
makes sense, github's windows runners don't have a wsl layer, we should think about that. |
geetu040
left a comment
geetu040
left a comment
There was a problem hiding this comment.
I see your current approach that pytest relies on the docker services, it would run them through the script and make sure they are running before starting the tests. And given that approach, your implementation is really good.
However I think the pytests should be independent of the docker-services. There could be an env variable that checks if it's on CI, then uses the local server, otherwise use the remote servers.
For now, we want to only get the docker services running in the github ci, we don't want to update the pytests yet. We'll see how long it takes, how reliable are the tests on this update and then apply these changes which should simply update the with_server function in conftest.py
| subprocess.run(["docker", "wait", "openml-test-setup-ci"], check=True) | ||
|
|
||
| @pytest.fixture(scope="session", autouse=True) | ||
| def openml_docker_stack(tmp_path_factory, worker_id): |
There was a problem hiding this comment.
please undo these changes. this is a really good implementation but we don't want to manually setup the docker services when pytests are run, that could be problematic for developers setting up pytests (it also clones the other repo inside this which is really bad), though we'll give them an option to setup their docker services themselves and toggle the pytest behaviour to use localhosts via an env variable
There was a problem hiding this comment.
it also clones the other repo inside this which is really bad
I am still reading all the reviews, but this does not happen locally, only in CI.
There was a problem hiding this comment.
there should be following steps to setup the docker services, though these should be ignored for window machines
- clone the repo, which you are already doing
- run the docker services
- verify that localhost endpoints are live
3 participants
Metadata
Details
This PR implements the setting up of the v1 and v2 test servers using docker via
localhostwheneverpytestis run, eventually we plan to migrate fully over to these tests instead of usingtest.openml.org.docker-compose.yml.docker/update.shfrom the server-api repository to setup the test server.test.ymlandconftest.pyto run the containers on CI and locally.