Skip to content

test(interop): update transport workflow and support unified-testing#2185

Merged
rlve merged 12 commits into
masterfrom
test-interop-1
Mar 26, 2026
Merged

test(interop): update transport workflow and support unified-testing#2185
rlve merged 12 commits into
masterfrom
test-interop-1

Conversation

@rlve

@rlve rlve commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Update Run Transport interoperability tests workflow to use latest master instead of pinned version. Use known-errors to mark existing issues and not fail the workflow.

Depends on: libp2p/test-plans#842

Tested against a fork rlve/unified-testing#1 for now. Currently python-v0.x (ws) is failing against nim-v1.15.

╲ Collecting results...
 ▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔
  → Results:
    → Total: 55
    ✓ Passed: 51
    ✗ Failed: 4
    - python-v0.x x nim-v1.15 (ws, noise, mplex)
    - python-v0.x x nim-v1.15 (ws, noise, yamux)
    - nim-v1.15 x python-v0.x (ws, noise, yamux)
    - nim-v1.15 x python-v0.x (ws, noise, mplex)
  → Total time: 00:16:34
  ✗ 4 test(s) failed

Affected Areas

  • Tests

@codecov-commenter

codecov-commenter commented Mar 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.61%. Comparing base (53c2275) to head (4d6eb0f).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2185      +/-   ##
==========================================
+ Coverage   72.52%   72.61%   +0.09%     
==========================================
  Files         155      155              
  Lines       19359    19398      +39     
  Branches       18       18              
==========================================
+ Hits        14040    14086      +46     
+ Misses       5319     5312       -7     

see 68 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rlve rlve changed the title test(interop): transport update workflow test(interop): update transport workflow Mar 24, 2026
Comment thread .github/workflows/interop.yml Outdated
Comment thread .github/workflows/interop.yml Outdated
@github-project-automation github-project-automation Bot moved this from new to In Progress in nim-libp2p Mar 24, 2026
@rlve rlve marked this pull request as ready for review March 26, 2026 11:20
@rlve rlve requested review from a team and Copilot March 26, 2026 11:20
@rlve rlve changed the title test(interop): update transport workflow test(interop): update transport workflow and support unified-testing Mar 26, 2026

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

Updates nim-libp2p’s transport interop setup to track newer upstream transport test tooling, and adds a v2 transport interop test app compatible with libp2p/unified-testing.

Changes:

  • Update transport interop GitHub Actions workflow to run the upstream test-plans transport action from master and configure known-errors/parallelism.
  • Add a new interop/transport-v2 Nim transport test app + Dockerfile emitting unified-testing compatible output.
  • Add minimal READMEs for transport interop apps.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
.github/workflows/interop.yml Switch transport interop workflow to use upstream action @master and configure worker-count + known-errors.
interop/transport/README.md Document purpose of the existing transport interop test app.
interop/transport-v2/main.nim New unified-testing-compatible transport interop test app (env-driven transport/muxer/security; redis coordination; outputs latency).
interop/transport-v2/Dockerfile Build/run container image for the new transport-v2 app.
interop/transport-v2/README.md Document purpose of the v2 transport interop test app.

run: docker buildx build --load -t nim-libp2p-transport-head -f interop/transport/Dockerfile .
- name: Run tests
uses: libp2p/test-plans/.github/actions/run-transport-interop-test@f58b7472d85aa053ce2ba8e9135bcc126a6fed3e
uses: libp2p/test-plans/.github/actions/run-transport-interop-test@master

Copilot AI Mar 26, 2026

Copy link

Choose a reason for hiding this comment

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

Referencing the test-plans action by @master makes CI non-deterministic (behavior can change without any change in this repo), which can cause flaky or surprising interop failures. Prefer pinning to a specific commit SHA or tagged release, and update the pin intentionally when you want new behavior.

Suggested change
uses: libp2p/test-plans/.github/actions/run-transport-interop-test@master
uses: libp2p/test-plans/.github/actions/run-transport-interop-test@4c0a5a8a8a2e0e9a7f4b3a0c9df2e4e8b7d1c2a3

Copilot uses AI. Check for mistakes.
Comment on lines +40 to +55
case transport
of "tcp":
discard switchBuilder.withTcpTransport().withAddress(
MultiAddress.init("/ip4/" & ip & "/tcp/0").tryGet()
)
of "quic-v1":
discard switchBuilder.withQuicTransport().withAddress(
MultiAddress.init("/ip4/" & ip & "/udp/0/quic-v1").tryGet()
)
of "ws":
discard switchBuilder.withWsTransport().withAddress(
MultiAddress.init("/ip4/" & ip & "/tcp/0/ws").tryGet()
)
else:
raiseAssert "unsupported transport"

Copilot AI Mar 26, 2026

Copy link

Choose a reason for hiding this comment

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

raiseAssert "unsupported transport" raises a Defect (not a CatchableError), so it will bypass the bottom-level except CatchableError handler and crash without the intended structured logging/exit code. Use a normal failure path here (e.g., quit with a message or raise a CatchableError/custom exception) so invalid/empty TRANSPORT is reported cleanly.

Copilot uses AI. Check for mistakes.
Comment on lines +32 to +36
redisAddr = getEnv("REDIS_ADDR", "redis:6379").split(":")

# using synchronous redis because async redis is based on
# asyncdispatch instead of chronos
redisClient = open(redisAddr[0], Port(parseInt(redisAddr[1])))

Copilot AI Mar 26, 2026

Copy link

Choose a reason for hiding this comment

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

getEnv("REDIS_ADDR", "redis:6379").split(":") is used without validating the split length, and redisAddr[1] can raise an IndexDefect (a Defect, not caught) if the env var is malformed (e.g., missing port or contains extra colons). Parse/validate REDIS_ADDR explicitly (and handle IPv6 bracket form if needed) before indexing.

Copilot uses AI. Check for mistakes.
Comment on lines +83 to +85
redisClient.bLPop(@[testKey & "_listener_multiaddr"], testTimeout.seconds.int)[
1
]

Copilot AI Mar 26, 2026

Copy link

Choose a reason for hiding this comment

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

bLPop(...)[1] assumes the Redis response always has at least 2 elements; on timeout or unexpected response it can raise an IndexDefect (not caught by the except CatchableError at the bottom). Check the returned sequence length / nil case and turn it into a controlled error (or retry) so timeouts surface as a clear test failure instead of a crash.

Suggested change
redisClient.bLPop(@[testKey & "_listener_multiaddr"], testTimeout.seconds.int)[
1
]
let resp = redisClient.bLPop(
@[testKey & "_listener_multiaddr"], testTimeout.seconds.int
)
if resp.len >= 2:
resp[1]
else:
raise newException(
CatchableError,
"bLPop returned empty or malformed response (len=" & $resp.len & ")"
)

Copilot uses AI. Check for mistakes.
Comment on lines +58 to +64
discard switchBuilder.withNoise()

case muxer
of "yamux":
discard switchBuilder.withYamux()
of "mplex":
discard switchBuilder.withMplex()

Copilot AI Mar 26, 2026

Copy link

Choose a reason for hiding this comment

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

The secureChannel/muxer cases have no else branches, so unexpected or empty env values silently leave the switch misconfigured (notably: an unrecognized MUXER leaves b.muxers empty, which causes outbound connection upgrade to fail later with a less actionable error). Consider validating these env vars explicitly and failing fast with a clear message when the value is unsupported.

Suggested change
discard switchBuilder.withNoise()
case muxer
of "yamux":
discard switchBuilder.withYamux()
of "mplex":
discard switchBuilder.withMplex()
discard switchBuilder.withNoise()
else:
raiseAssert "unsupported secure channel"
case muxer
of "yamux":
discard switchBuilder.withYamux()
of "mplex":
discard switchBuilder.withMplex()
else:
raiseAssert "unsupported muxer"

Copilot uses AI. Check for mistakes.
@rlve rlve merged commit 3463ddb into master Mar 26, 2026
40 of 41 checks passed
@rlve rlve deleted the test-interop-1 branch March 26, 2026 14:20
@github-project-automation github-project-automation Bot moved this from In Progress to done in nim-libp2p Mar 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants