Improve v2 SDK test robustness and failure diagnostics

[RafaelPChequer]

Description

Improved end-to-end tests for v2 WebRTC in the SDK by adding a helper function to detect MEDIA_TIMEOUT and unexpected hangups, implementing periodic checks during call duration to ensure calls remain active and catch failures early, and enhancing audio reception validation with fallbacks for missing metrics (e.g., using packetsReceived or totalSamplesReceived when totalAudioEnergy is unavailable, common with G.711 codecs). These changes increase test reliability, provide better error diagnostics for network/ICE issues, and help prevent regressions in SPR-related scenarios.

Type of change

• Internal refactoring
• Bug fix (bugfix - non-breaking)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Code snippets

In case of new feature or breaking changes, please include code snippets.

// Helper to detect MEDIA_TIMEOUT and unexpected hangups
const expectCallNotHungUp = async (page: Page, callName: string) => {
  const callStatus = page.locator('#callStatus')
  const statusText = await callStatus.textContent()

  if (statusText?.includes('MEDIA_TIMEOUT')) {
    throw new Error(
      `${callName} failed with MEDIA_TIMEOUT (Code 804). ` +
      `This usually indicates:\n` +
      `- TURN server not available or misconfigured\n` +
      `- Firewall blocking UDP/TURN traffic (port 3478)\n` +
      `- Network instability or high latency\n` +
      `- ICE gathering failed\n` +
      `Status: ${statusText}`
    )
  }

  if (statusText?.includes('hangup') || statusText?.includes('destroy')) {
    throw new Error(
      `${callName} unexpectedly hung up during test. Status: ${statusText}`
    )
  }

  await expect(callStatus).toContainText('-> active')
}

const callDurationMs = 40000
const checkInterval = 5000
const numChecks = Math.floor(callDurationMs / checkInterval)

console.log(`Monitoring calls for ${callDurationMs}ms (checking every ${checkInterval}ms)...`)

for (let i = 0; i < numChecks; i++) {
  await pageCallee1.waitForTimeout(checkInterval)

  // Verify both calls are still active (detect MEDIA_TIMEOUT early)
  await Promise.all([
    expectCallNotHungUp(pageCallee1, 'Callee1'),
    expectCallNotHungUp(pageCallee2, 'Callee2')
  ])

  console.log(`✓ Calls still active after ${(i + 1) * checkInterval}ms`)
}

const inboundRtp = audioStats['inbound-rtp']
const totalAudioEnergy = inboundRtp?.totalAudioEnergy
const packetsReceived = inboundRtp?.packetsReceived
const totalSamplesReceived = inboundRtp?.totalSamplesReceived

// Validate that at least one metric is available
if (!inboundRtp || (!totalAudioEnergy && !packetsReceived && !totalSamplesReceived)) {
  throw new Error(
    'No audio metrics available in RTCStats report. ' +
    'This indicates a problem with the WebRTC connection or RTCStats API. ' +
    `Stats: ${JSON.stringify(audioStats)}`
  )
}

// Prefer totalAudioEnergy if available
if (totalAudioEnergy !== undefined && totalAudioEnergy !== null) {
  expect(totalAudioEnergy).toBeGreaterThan(minTotalAudioEnergy)
  console.log(`✓ Audio energy validated: ${totalAudioEnergy} > ${minTotalAudioEnergy}`)
} else {
  // Fallback to packetsReceived (common with G.711 codecs)
  console.log(
    '⚠️ totalAudioEnergy not available (common with G.711/PCMU/PCMA codecs), ' +
    'using packetsReceived as fallback validation'
  )

  if (packetsReceived !== undefined && packetsReceived !== null) {
    expect(packetsReceived).toBeGreaterThan(minPacketsReceived)
    console.log(`✓ Packets validated: ${packetsReceived} > ${minPacketsReceived}`)
  } else {
    throw new Error(
      'Neither totalAudioEnergy nor packetsReceived available. ' +
      'Cannot validate audio reception. ' +
      `Available metrics: ${JSON.stringify(inboundRtp)}`
    )
  }
}

[changeset-bot]

⚠️ No Changeset found

Latest commit: 9097e21

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[giavac]

What made you add this comment (and the related following one,

// Fallback to packetsReceived (common with G.711 codecs)

)?

[giavac]

Thanks @RafaelPChequer - I've left a few suggestions/comments.

[iAmmar7]

The v2 SDK tests were moved to the rtc-testing repo. If you’re improving these tests, please check with Giacomo on whether to make them here or in the rtc-testing repo.

[claude]

Claude finished @RafaelPChequer's task —— View job

---

Code Review Complete ✅

I've thoroughly analyzed this PR and provide the following comprehensive review:

📋 Summary

This PR significantly enhances the robustness and diagnostic capabilities of v2 WebRTC e2e tests. The improvements focus on early failure detection, better error messaging, and fallback validation mechanisms for different codec scenarios.

✅ Strengths

1. Excellent Error Diagnostics

// v2WebrtcFromRest.spec.ts:334-348
const expectCallNotHungUp = async (page: Page, callName: string) => {
  if (statusText?.includes('MEDIA_TIMEOUT')) {
    throw new Error(
      `${callName} failed with MEDIA_TIMEOUT (Code 804). ` +
      `This usually indicates:\n` +
      `- TURN server not available or misconfigured\n` +
      `- Firewall blocking UDP/TURN traffic (port 3478)\n` +
      `- Network instability or high latency\n` +
      `- ICE gathering failed\n`
    )
  }
}

✅ Excellent: Provides actionable debugging information for common WebRTC issues.

2. Proactive Monitoring

// v2WebrtcFromRest.spec.ts:427-443
const callDurationMs = 40000
const checkInterval = 5000
for (let i = 0; i < numChecks; i++) {
  await pageCallee1.waitForTimeout(checkInterval)
  await Promise.all([
    expectCallNotHungUp(pageCallee1, 'Callee1'),
    expectCallNotHungUp(pageCallee2, 'Callee2')
  ])
}

✅ Smart: Catches failures early instead of waiting until the end.

3. Robust Audio Validation

// utils.ts:1216-1250
if (totalAudioEnergy !== undefined && totalAudioEnergy !== null) {
  expect(totalAudioEnergy).toBeGreaterThan(minTotalAudioEnergy)
} else {
  console.log('⚠️ totalAudioEnergy not available (common with G.711/PCMU/PCMA codecs)')
  if (packetsReceived !== undefined && packetsReceived !== null) {
    expect(packetsReceived).toBeGreaterThan(minPacketsReceived)
  }
}

✅ Practical: Handles codec-specific metric variations gracefully.

⚠️ Areas for Improvement

1. Code Duplication

🔍 Issue: expectCallActive and expectCallHangup functions are duplicated across multiple test cases.
📝 Recommendation: Extract these to utils.ts to reduce duplication and improve maintainability.

2. Magic Numbers

🔍 Issue: Hard-coded values like callDurationMs = 40000, checkInterval = 5000.
📝 Recommendation: Extract to constants or configuration:

const TEST_CONFIG = {
  CALL_DURATION_MS: 40000,
  HEALTH_CHECK_INTERVAL_MS: 5000,
  MIN_AUDIO_ENERGY_RATIO: 1/50000,
} as const

3. Error Message Formatting

🔍 Issue: Long concatenated error strings are hard to maintain.
📝 Recommendation: Use template literals or helper functions:

const createMediaTimeoutError = (callName: string, statusText: string) => 
  new Error(`${callName} failed with MEDIA_TIMEOUT (Code 804).
This usually indicates:
- TURN server not available or misconfigured
- Firewall blocking UDP/TURN traffic (port 3478)
- Network instability or high latency
- ICE gathering failed
Status: ${statusText}`)

🐛 Potential Issues

1. Race Condition Risk

🔍 Location: v2WebrtcFromRest.spec.ts:433-443

for (let i = 0; i < numChecks; i++) {
  await pageCallee1.waitForTimeout(checkInterval) // Only waits on page1
  await Promise.all([...]) // Checks both pages
}

📝 Recommendation: Wait on both pages or use a shared timer to avoid timing inconsistencies.

2. Type Safety

🔍 Issue: Extensive use of any types in stats processing.
📝 Recommendation: Define proper interfaces for RTCStats reports to improve type safety.

🚀 Performance Considerations

✅ Good: Parallel execution using Promise.all() for multiple operations
✅ Reasonable: 5-second monitoring intervals balance detection speed vs. test overhead
⚠️ Watch: Audio stats collection frequency could impact performance in large test suites

🔒 Security Assessment

✅ No security concerns: Test code with appropriate environment variable usage
✅ Safe: Proper error handling without sensitive data exposure

📊 Test Coverage & Quality

✅ Comprehensive: Covers multiple failure scenarios (MEDIA_TIMEOUT, unexpected hangups)
✅ Resilient: Fallback validation mechanisms for different codec behaviors
✅ Informative: Detailed logging for debugging test failures

🔧 Addressing Reviewer Comments

@giavac's suggestions:

1. ✅ Add FS bug reference: The suggestion to add "An old bug in FS - check version and environment" is valid for historical context.
2. ✅ Formatting consistency: Using "ms" instead of "ms" for consistency.
3. ❓ G.711 codec comment: This appears to be based on empirical testing where totalAudioEnergy is unavailable with G.711 codecs, requiring fallback to packet-based validation.

@iAmmar7's concern:
⚠️ Important: The note about v2 SDK tests potentially moving to rtc-testing repo should be clarified before merging.

📝 Missing Changeset

⚠️ Action Required: The changeset bot indicates no changeset was added. Since this improves test infrastructure, consider adding a changeset with:

npm run changeset

Select "patch" for internal testing improvements.

🎯 Final Recommendation

APPROVE with minor improvements. This PR significantly enhances test reliability and debugging capabilities. The proactive monitoring and detailed error diagnostics will help developers identify and resolve WebRTC issues faster.

Priority fixes:

1. Extract duplicate helper functions to utils
2. Add changeset for tracking
3. Consider creating constants for magic numbers

The core improvements are valuable and the code quality is good overall.

---