Skip to content

test(whatsapp): cover npm install timeout#26795

Open
haran2001 wants to merge 1 commit into
NousResearch:mainfrom
haran2001:fix/whatsapp-npm-install-timeout
Open

test(whatsapp): cover npm install timeout#26795
haran2001 wants to merge 1 commit into
NousResearch:mainfrom
haran2001:fix/whatsapp-npm-install-timeout

Conversation

@haran2001

@haran2001 haran2001 commented May 16, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

The WhatsApp gateway bridge runs npm install to set up its Node dependencies on first connect. That call previously used the default 60s subprocess timeout, which fired on slower NAS/container hosts and aborted the install partway through. The default was raised to 300s and an env override (WHATSAPP_NPM_INSTALL_TIMEOUT) was added; this PR adds regression coverage so neither change can silently regress.

Related Issue

Fixes #14980

Type of Change

  • ✅ Tests (adding or improving test coverage)

Changes Made

  • tests/gateway/test_whatsapp_connect.py:
    • Extract the node_modules-missing predicate into a shared TestConnectCleanup static helper to deduplicate setup across the new tests.
    • Add test_npm_install_uses_default_timeout — asserts subprocess.run is called with timeout=300 when WHATSAPP_NPM_INSTALL_TIMEOUT is unset.
    • Add test_npm_install_honors_env_override — asserts WHATSAPP_NPM_INSTALL_TIMEOUT=600 overrides the default and is passed through to subprocess.run.

How to Test

  1. python -m pytest tests/gateway/test_whatsapp_connect.py -q -o 'addopts='
  2. Both new tests pass; existing tests in the file are unaffected.
  3. git diff --check is clean.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (test(whatsapp):)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the targeted test file and it passes
  • I've added tests for my changes (this PR is the tests)
  • I've tested on my platform: macOS 15.6.1

Documentation & Housekeeping

  • I've updated relevant documentation — N/A (test-only)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) — tests stub subprocess.run, no platform-specific behavior
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

$ python -m pytest tests/gateway/test_whatsapp_connect.py -q -o 'addopts='
... all tests pass ...

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have platform/whatsapp WhatsApp Business adapter labels May 16, 2026
@austinpickett austinpickett requested a review from Copilot May 18, 2026 14:56

@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.

Please use PULL_REQUEST_TEMPLATE.md

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds regression test coverage for the WhatsApp bridge npm install timeout, verifying that the new 300s default is in effect and the WHATSAPP_NPM_INSTALL_TIMEOUT env override is honored.

Changes:

  • Extracts the node_modules-missing path predicate into a shared static helper on TestConnectCleanup.
  • Adds a test confirming the default subprocess.run timeout is 300 seconds when the env var is unset.
  • Adds a test confirming WHATSAPP_NPM_INSTALL_TIMEOUT=600 overrides the default.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@haran2001

Copy link
Copy Markdown
Contributor Author

Reformatted the description to match PULL_REQUEST_TEMPLATE.md — sections (What does this PR do? / Related Issue / Type of Change / Changes Made / How to Test / Checklist) are all populated. Ready for another look, thanks!

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 platform/whatsapp WhatsApp Business adapter type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Setup]: WhatsApp bridge npm install timeout too short (60s) on container startup

4 participants