Skip to content

fix(tests): use platform-safe timeout method#43182

Closed
Ghraven wants to merge 1 commit into
NousResearch:mainfrom
Ghraven:fix-windows-pytest-timeout-method
Closed

fix(tests): use platform-safe timeout method#43182
Ghraven wants to merge 1 commit into
NousResearch:mainfrom
Ghraven:fix-windows-pytest-timeout-method

Conversation

@Ghraven

@Ghraven Ghraven commented Jun 10, 2026

Copy link
Copy Markdown

Problem

Running the pytest suite on native Windows can fail before tests execute because pyproject.toml forces pytest-timeout to use --timeout-method=signal. Windows does not expose signal.SIGALRM, so pytest exits with an internal error.

Before / after

Before, commands such as:

python -m pytest tests\hermes_cli\test_inventory.py -q

failed on Windows with:

AttributeError: module 'signal' has no attribute 'SIGALRM'

After, the timeout method is left unset so pytest-timeout can choose the platform-safe default. POSIX can still use signal behavior, while Windows can fall back to its supported method.

Verification

.\venv\Scripts\python.exe -m pytest tests\hermes_cli\test_inventory.py -q
.\venv\Scripts\python.exe -m py_compile hermes_cli\inventory.py

Result: 23 passed.

@Ghraven Ghraven requested a review from a team June 10, 2026 00:21
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have labels Jun 10, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Thanks for this! This is already fixed on main — PR #39881 switched --timeout-method to thread (the cross-platform default), which resolves the Windows SIGALRM collection crash (#39880). The current pyproject.toml already has addopts = "-m 'not integration' --timeout=30 --timeout-method=thread", so this change is a duplicate of merged work. Closing as duplicate of #39881.

@Ghraven

Ghraven commented Jun 10, 2026

Copy link
Copy Markdown
Author

Thanks for checking, @alt-glitch — you're right, I missed that #39881 already landed the thread timeout method on main. Closing makes sense. 👍

@austinpickett austinpickett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix is correct and complete. Dropping from lets auto-select the platform-appropriate method:

  • POSIX: still defaults to — no behavioral regression
  • Windows: falls back to , which avoids the crash on native Windows runs

The change is a single-line removal in the right place. The PR body includes a concrete before/after error trace and a verified 23-passed Windows test run. Nothing hidden here.

LGTM. ✅

@austinpickett austinpickett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix is correct and complete. Dropping the explicit timeout-method=signal from pyproject.toml addopts lets pytest-timeout auto-select the platform-appropriate method: POSIX still defaults to signal (no behavioral regression), while Windows falls back to 'thread', which avoids the AttributeError crash on native Windows runs (signal.SIGALRM does not exist on Windows).

The change is a single-line removal in exactly the right place. The PR body includes a concrete before/after error trace and a verified 23-passed Windows test run.

LGTM. ✅

@Ghraven

Ghraven commented Jun 11, 2026

Copy link
Copy Markdown
Author

Thanks again for the review and for checking this. Since the same fix already landed on main in #39881, I’ll close this to avoid adding noise. I appreciate the time you both spent looking at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants