Add integration tests on a self-hosted runner (#104)

* Add integration tests on a self-hosted runner Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Fix import ordering issues Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * attempt to fix test_check_passwordless Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Adding XFAILS (very bad) bc of Paramiko Banner bug Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Add early exit path in `check_passwordless` Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Add xfail on `mila code` integration test ;( Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Try to fix tests in github CI Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Fix tests in test_init_command Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Adapt tests for `mila init` for self-hosted runner Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Add missing regression files Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Fix condition on `test_check_passwordless` Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Fix check for macos test of check_passwordless Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Adjust the unit-tests workflow for macos Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Revert "Adjust the unit-tests workflow for macos" This reverts commit f40dfc4. * Add SSH config file check in check_passwordless Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Adjust test_check_passwordless for self-hosted run Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Update check for Paramiko OpenSSH key parsing bug Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Remove redundant comment Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Fix flaky tests for parallel progress bar timing Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Fix test_vscode_installed for self-hosted runner Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Add very ugly mark on test_check_passwordless Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Remove duplicate marks on `test_check_passwordless` Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Simplify test_parallel_progress_bar Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Increase the timeout value to 30 minutes in CI Signed-off-by: Fabrice Normandin <normandf@mila.quebec> * Add broad xfail for `check_passwordless` Signed-off-by: Fabrice Normandin <normandf@mila.quebec> --------- Signed-off-by: Fabrice Normandin <normandf@mila.quebec>
mila-iqia · Apr 11, 2024 · 5adb85c · 5adb85c
1 parent 1518e01
commit 5adb85c
Show file tree

Hide file tree

Showing 20 changed files with 633 additions and 267 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -68,8 +68,8 @@ jobs:
         name: codecov-umbrella
         fail_ci_if_error: false
 
-  integration-tests:
-    name: integration tests
+  mock-slurm-integration-tests:
+    name: integration tests with a mock slurm cluster
     needs: [unit-tests]
     runs-on: ${{ matrix.platform }}
 
@@ -128,8 +128,8 @@ jobs:
           poetry install --with=dev
 
       - name: Launch integration tests
-        run: poetry run pytest tests/integration --cov=milatools --cov-report=xml --cov-append -s -vvv --log-level=DEBUG
-        timeout-minutes: 3
+        run: poetry run pytest --slow --cov=milatools --cov-report=xml --cov-append -vvv --log-level=DEBUG
+        timeout-minutes: 5
         env:
           SLURM_CLUSTER: localhost
 
@@ -141,3 +141,45 @@ jobs:
           env_vars: PLATFORM,PYTHON
           name: codecov-umbrella
           fail_ci_if_error: false
+
+  real-slurm-integration-tests:
+    name: integration tests with a real SLURM cluster
+    needs: [mock-slurm-integration-tests]
+    runs-on: self-hosted
+
+    strategy:
+      max-parallel: 5
+      matrix:
+        # TODO: We should ideally also run this with Windows/Mac clients and a Linux
+        # server. Unsure how to set that up with GitHub Actions though.
+        python-version: ['3.8', '3.9', '3.10', '3.11']
+
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Set up Python ${{ matrix.python-version }}
+        uses: actions/setup-python@v5
+        with:
+          python-version: ${{ matrix.python-version }}
+
+      - name: Install dependencies
+        run: |
+          python -m pip install --upgrade pip
+          pip install poetry
+          poetry install --with=dev
+
+      - name: Launch integration tests
+        id: self_hosted_integration_tests
+        run: poetry run pytest --slow --cov=milatools --cov-report=xml --cov-append -vvv --log-level=DEBUG
+        timeout-minutes: 30
+        env:
+          SLURM_CLUSTER: mila
+
+      - name: Upload coverage reports to Codecov
+        uses: codecov/codecov-action@v3
+        with:
+          file: ./coverage.xml
+          flags: integrationtests
+          env_vars: PLATFORM,PYTHON
+          name: codecov-umbrella
+          fail_ci_if_error: false
diff --git a/milatools/cli/local.py b/milatools/cli/local.py
@@ -3,6 +3,7 @@
 import shlex
 import socket
 import subprocess
+import sys
 from logging import getLogger as get_logger
 from subprocess import CompletedProcess
 from typing import IO, Any
@@ -11,6 +12,8 @@
 import paramiko.ssh_exception
 from typing_extensions import deprecated
 
+from milatools.utils.remote_v2 import SSH_CONFIG_FILE, is_already_logged_in
+
 from .utils import CommandNotFoundError, T, cluster_to_connect_kwargs
 
 logger = get_logger(__name__)
@@ -79,6 +82,13 @@ def display(split_command: list[str] | tuple[str, ...] | str) -> None:
 
 
 def check_passwordless(host: str) -> bool:
+    if (
+        sys.platform != "win32"
+        and SSH_CONFIG_FILE.exists()
+        and is_already_logged_in(host)
+    ):
+        return True
+
     try:
         connect_kwargs_for_host = {"allow_agent": False}
         if host in cluster_to_connect_kwargs:

diff --git a/milatools/cli/remote.py b/milatools/cli/remote.py
@@ -128,7 +128,7 @@ def __init__(
                     _connect_kwargs.update(connect_kwargs)
 
                 logger.info(f"Opening a new connection to {hostname}")
-                logger.debug(f"Connection kwargs: {_connect_kwargs}")
+                logger.debug(f"connect_kwargs: {_connect_kwargs}")
                 connection = Connection(hostname, connect_kwargs=_connect_kwargs)
                 if keepalive:
                     connection.open()

diff --git a/milatools/utils/remote_v2.py b/milatools/utils/remote_v2.py
@@ -193,7 +193,9 @@ def is_already_logged_in(cluster: str, also_run_command_to_check: bool = False)
         100% sure that we are logged in. In most cases this isn't necessary so we can
         skip it, since it can take a few seconds.
     """
-    control_path = get_controlpath_for(cluster)
+    if not SSH_CONFIG_FILE.exists():
+        return False
+    control_path = get_controlpath_for(cluster, ssh_config_path=SSH_CONFIG_FILE)
     if not control_path.exists():
         logger.debug(f"ControlPath at {control_path} doesn't exist. Not logged in.")
         return False

diff --git a/poetry.lock b/poetry.lock
diff --git a/pyproject.toml b/pyproject.toml
@@ -18,34 +18,24 @@ fabric = "^3.2.2"
 tqdm = "^4.66.1"
 rich = "^13.7.0"
 
-[tool.poetry.dev-dependencies]
-black = ">= 21.8b0"
-isort = ">= 5.9.3"
-flake8 = {version = ">= 6.0.0", python = "^3.8.1"}
-Sphinx = "^5.0.1"
-sphinx-rtd-theme = "^1.0.0"
-pytest = ">=7.2.0"
-pytest-cov = ">=2.8.1"
-coverage = {extras = ["toml"], version = "^5.0.3"}
-toml = "^0.10.0"
-
 [tool.poetry.scripts]
 mila = "milatools.cli.__main__:main"
 
 [tool.poetry.group.dev.dependencies]
-pytest = "^7.2.1"
-pytest-regressions = "^2.4.2"
+black = ">= 21.8b0"
+coverage = {extras = ["toml"], version = "^5.0.3"}
 fabric = {extras = ["testing"], version = "^3.2.2"}
+flake8 = {version = ">= 6.0.0", python = "^3.8.1"}
+pytest = "^7.2.1"
+pytest-cov = "^4.1.0"
 pytest-mock = "^3.11.1"
+pytest-regressions = "^2.4.2"
+pytest-skip-slow = "^0.0.5"
 pytest-socket = "^0.6.0"
-pytest-cov = "^4.1.0"
 pytest-timeout = "^2.2.0"
-
-
-[tool.isort]
-multi_line_output = 3
-include_trailing_comma = true
-combine_as_imports = true
+Sphinx = "^5.0.1"
+sphinx-rtd-theme = "^1.0.0"
+toml = "^0.10.0"
 
 
 [tool.pytest.ini_options]

diff --git a/tests/cli/common.py b/tests/cli/common.py
@@ -20,13 +20,20 @@
 from milatools.utils.remote_v2 import RemoteV2
 
 if typing.TYPE_CHECKING:
-    from typing import TypeGuard
+    from typing_extensions import TypeGuard
 
-in_github_CI = all(var in os.environ for var in ["CI", "GITHUB_ACTION", "GITHUB_ENV"])
+in_github_CI = os.environ.get("GITHUB_ACTIONS") == "true"
 """True if this is being run inside the GitHub CI."""
 
+# See https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables
+in_self_hosted_github_CI = (
+    in_github_CI and os.environ.get("GITHUB_ACTION") == "self_hosted_integration_tests"
+)
+
+
 skip_if_on_github_CI = pytest.mark.skipif(
-    in_github_CI, reason="This test shouldn't run on the Github CI."
+    in_github_CI and not in_self_hosted_github_CI,
+    reason="This test shouldn't run on the Github CI.",
 )
 skip_param_if_on_github_ci = functools.partial(pytest.param, marks=skip_if_on_github_CI)