js-libp2p icon indicating copy to clipboard operation
js-libp2p copied to clipboard

Move WebRTC stream timeline and flag handling tests to interface compliance suite for all transports

Open lalitcap23 opened this issue 1 month ago • 5 comments

Description

There are three test suites in packages/transport-webrtc/test/stream.spec.ts that are currently skipped with describe.skip and have TODO comments indicating they should be moved to the transport interface compliance test suite.

These tests validate stream behavior that should be consistent across ALL transport implementations (TCP, WebRTC, WebSockets, WebTransport, etc.), not just WebRTC.

Current Behavior

The following test suites are skipped in packages/transport-webrtc/test/stream.spec.ts:

  1. Stream Stats (line 120)

    • Tests stream timeline tracking (open/close)
    • Tests write status transitions
    • Tests read status transitions
  2. Stream Read Stats Transition By Incoming Flag (line 188)

    • Tests how streams handle incoming FIN flags
    • Tests how streams handle STOP_SENDING flags
  3. Stream Write Stats Transition By Incoming Flag (line 231)

    • Tests how streams handle outgoing close flags

Each has a comment:

// TODO: move to transport interface compliance suite
describe.skip('Stream Stats', () => {

lalitcap23 avatar Nov 24 '25 14:11 lalitcap23

can i take this issue to resolve .

lalitcap23 avatar Nov 24 '25 14:11 lalitcap23

Yes, feel free to open a PR. Double check that they haven't already been re-implemented. It's possible these tests, or their equivalent already exist in the compliance suite.

tabcat avatar Nov 24 '25 14:11 tabcat

@tabcat , Thanks for the feedback! I've done a careful review comparing the WebRTC tests with the compliance suite.

Findings: Already in compliance suite:

  • Stream timeline tracking (open/close)
  • Basic close and abort behavior
  • Write/read status on close

Missing from compliance suite:

  • Isolated closeRead() behavior test (stream should be write-open, read-closed)
  • onRemoteReset() behavior test
  • WebRTC-specific flag handling (FIN, STOP_SENDING flags) The WebRTC flag tests (2nd and 3rd suites) are WebRTC data channel protocol-specific and probably shouldn't be in the generic compliance suite.

The first suite has a few tests that could be generalized (closeRead, reset behavior) but most overlap with existing compliance tests. Recommendation: Should we: -Extract just the closeRead() and reset tests to compliance suite? -Unskip and keep all WebRTC tests as protocol-specific? -Close as duplicate since the core behavior is already tested?

lalitcap23 avatar Nov 24 '25 15:11 lalitcap23

The closeRead and reset tests could be moved to the general compliance. And the todo comment can be deleted. The WebRTC specific tests aren't touched.

tabcat avatar Nov 24 '25 17:11 tabcat

Hi @tabcat , could you review this PR and let me know if any changes are needed?

lalitcap23 avatar Nov 26 '25 07:11 lalitcap23