Conversation
ssbarnea
force-pushed
the
feat/adt-server-testing
branch
from
6a8b844 to
e134128
Compare
ssbarnea
force-pushed
the
feat/adt-server-testing
branch
from
e134128 to
674dd21
Compare
ssbarnea
force-pushed
the
feat/adt-server-testing
branch
from
674dd21 to
a11b9bf
Compare
ssbarnea
force-pushed
the
feat/adt-server-testing
branch
from
a11b9bf to
a9284d0
Compare
ssbarnea
force-pushed
the
feat/adt-server-testing
branch
from
a9284d0 to
3f16e1f
Compare
ssbarnea
force-pushed
the
feat/adt-server-testing
branch
from
3f16e1f to
9636f0d
Compare
There was a problem hiding this comment.
Pull request overview
Refactors test infrastructure bootstrapping for the ADT server to unify container/non-container startup, capture server output to log files, and adjust pytest warning/verbosity and coverage-related behavior.
Changes:
- Replace hard-coded server ports with shared constants and reuse them across fixtures/startup logic.
- Consolidate container and host server startup into a single
_start_server(container=...)code path and log server output to per-mode log files. - Update pytest warning filtering to avoid failing the run on
pytest.PytestWarning.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/ansible_dev_tools/tests/conftest.py |
Refactors server/container startup and URL/port handling; adds log capture for server processes. |
pyproject.toml |
Adjusts pytest warning filters to always emit (and not error on) pytest.PytestWarning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert INFRASTRUCTURE is not None | ||
| bin_path = shutil.which("adt") | ||
| if bin_path is None: | ||
| msg = "adt not found in $PATH" | ||
| raise RuntimeError(msg) | ||
| server_log_file = Path(os.environ.get("TOX_ENV_DIR", ".tox")) / "log" / "server.log" | ||
| log_file_name = "server.log" if not container else "container-server.log" | ||
| server_log_file = Path(os.environ.get("TOX_ENV_DIR", ".tox")) / "log" / log_file_name | ||
|
|
||
| server_log_file.parent.mkdir(parents=True, exist_ok=True) | ||
| LOGGER.warning("Starting adt server with log file at %s", server_log_file) | ||
| cmd = ( | ||
| f"{bin_path} server -p {ADT_SERVER_PORT} --debug" if not container else _get_container_cmd() | ||
| ) |
There was a problem hiding this comment.
_start_server unconditionally requires adt to be present on the host (bin_path = shutil.which("adt")), but in the container=True path the command is built from _get_container_cmd() and runs adt server inside the container. This means --only-container runs will fail unnecessarily if adt isn't installed on the host. Consider only resolving/validating bin_path when container is false.
| if INFRASTRUCTURE.container: | ||
| _start_server(container=True) | ||
| if INFRASTRUCTURE.server: | ||
| _start_server(container=False) | ||
| if not INFRASTRUCTURE.container and not INFRASTRUCTURE.server: | ||
| err = "Cannot run tests with any server option." | ||
| pytest.exit(err, 2) |
There was a problem hiding this comment.
In --include-container mode, Infrastructure.__post_init__ sets both container=True and server=True, so pytest_sessionstart calls _start_server() twice. Since _start_server stores the process in INFRASTRUCTURE.proc, the second call overwrites the first process handle, so the container server process is no longer tracked (and readiness checks/logging can be misleading). Consider keeping separate process slots (e.g., proc_host/proc_container) or restoring separate _start_container() / _start_server() responsibilities so both servers can be started and stopped reliably in the same run.
| if INFRASTRUCTURE.server: | ||
| _start_server(container=False) | ||
| if not INFRASTRUCTURE.container and not INFRASTRUCTURE.server: | ||
| err = "Cannot run tests with any server option." |
There was a problem hiding this comment.
The message Cannot run tests with any server option. reads like tests cannot run when a server option is provided; it likely meant "without any server option" (or similar).
| err = "Cannot run tests with any server option." | |
| err = "Cannot run tests without any server option." |
| def _get_container_cmd() -> str: | ||
| """Start the container. | ||
|
|
||
| The default image for navigator is pulled ahead of time. |
There was a problem hiding this comment.
_get_container_cmd() now returns the container run command (it doesn't actually start the container), but the docstring still starts with "Start the container." This is misleading for readers and for future refactors; consider updating the summary line to reflect that it builds the command (and performs any required preflight steps) rather than starting it.
| else: | ||
| nav_ee = get_nav_default_ee_in_container() | ||
| _proc = _exec_container(command=f"podman pull {nav_ee}") | ||
| return cmd | ||
|
|
||
|
|
||
| def get_nav_default_ee_in_container() -> str: |
There was a problem hiding this comment.
_get_container_cmd() calls _exec_container(...) (e.g., podman load / podman pull) before the container is started, but _exec_container uses <engine> exec ... which requires the container to already be running. With the new flow (starting the container via subprocess.Popen(cmd, shell=True) in _start_server(container=True)), these exec calls will fail. Consider moving these operations to after the container is launched and healthy (or run them on the host with subprocess.run instead of exec).
| @@ -512,7 +524,7 @@ | |||
| while tries < max_tries: | |||
| try: | |||
| # timeout increased to 2s due to observed GHA macos failures | |||
| res = requests.get("http://localhost:8000", timeout=timeout) | |||
| res = requests.get(f"http://localhost:{ADT_SERVER_PORT}", timeout=timeout) | |||
| if res.status_code == requests.codes.get("not_found"): | |||
| return | |||
There was a problem hiding this comment.
When container=True, the server runs on ADT_SERVER_CONTAINER_PORT (8001), but the readiness probe always polls http://localhost:{ADT_SERVER_PORT} (8000). This can falsely report success (if the host server is up) while the container server is down. The probe URL should switch based on container to check the correct port.
| cmd = ( | ||
| f"{bin_path} server -p {ADT_SERVER_PORT} --debug" if not container else _get_container_cmd() | ||
| ) | ||
| msg = f"Starting adt server with `{cmd}` and log file at {server_log_file}" | ||
| LOGGER.warning(msg) | ||
| start_time = time.time() | ||
| with server_log_file.open("w") as log_file: | ||
| INFRASTRUCTURE.proc = subprocess.Popen( # noqa: S603 | ||
| [bin_path, "server", "-p", "8000", "--debug"], | ||
| INFRASTRUCTURE.proc = subprocess.Popen( | ||
| cmd, | ||
| env=os.environ, | ||
| shell=True, | ||
| stdout=log_file, | ||
| stderr=subprocess.STDOUT, | ||
| ) |
There was a problem hiding this comment.
For the non-container case, _start_server now builds a single shell command string and runs it with shell=True. This is less robust than passing an argv list (e.g., paths with spaces, quoting, platform differences) and makes the container/non-container code paths harder to reason about. Consider keeping shell=False + argv list for the host server, and only using a shell command string for the container engine invocation.
| cache_dir = "./.cache/.pytest" | ||
| filterwarnings = [ | ||
| # We raise one non critical warning from our own conftest.py: | ||
| "always::pytest.PytestWarning", |
There was a problem hiding this comment.
Adding "always::pytest.PytestWarning" before "error" means all PytestWarnings will no longer fail the test run, which can hide genuinely actionable pytest configuration warnings. If the goal is to silence a single known warning, it would be safer to filter that specific warning more narrowly (by message/module) instead of exempting the entire PytestWarning category.
| "always::pytest.PytestWarning", | |
| "always::pytest.PytestWarning:ansible_dev_tools.tests.conftest", |
ssbarnea
force-pushed
the
feat/adt-server-testing
branch
from
9636f0d to
ba99b1e
Compare
Uh oh!
There was an error while loading. Please reload this page.