test(interop): update transport script for unified-testing#2190
Conversation
rlve
requested review from
a team,
Copilot,
gmelodie,
richard-ramos and
vladopajic
There was a problem hiding this comment.
Pull request overview
Updates the transport interoperability test app and CI workflow to align nim-libp2p’s transport interop with libp2p’s newer unified-testing conventions.
Changes:
- Update
interop/transport/main.nimto use unified-testing environment variables, namespaced Redis keys, and a new stdout result format. - Disable the legacy
run-transport-interopGitHub Actions job (pending unified-testing support for implementation-side actions).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| interop/transport/main.nim | Switches to unified-testing env var names, uses TEST_KEY for Redis key namespacing, and changes result output format. |
| .github/workflows/interop.yml | Comments out the legacy transport interop job while leaving other interop jobs intact. |
| isDialer = getEnv("IS_DIALER") == "true" | ||
| testKey = getEnv("TEST_KEY") | ||
| envIp = getEnv("LISTENER_IP", "0.0.0.0") |
There was a problem hiding this comment.
TEST_KEY is used to namespace the Redis list key, but getEnv("TEST_KEY") defaults to an empty string when unset. If it’s missing, this app will read/write to the shared key _listener_multiaddr, which can cause cross-test interference when multiple transport tests run concurrently. Consider validating that TEST_KEY is non-empty at startup (and quitting with a clear error) before using it to build Redis keys.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2190 +/- ##
==========================================
+ Coverage 72.64% 72.69% +0.04%
==========================================
Files 155 155
Lines 19279 19279
Branches 19 19
==========================================
+ Hits 14006 14015 +9
+ Misses 5273 5264 -9 🚀 New features to boost your workflow:
|
| uses: jlumbroso/free-disk-space@v1.3.1 | ||
| with: | ||
| tool-cache: true | ||
| # run-transport-interop: |
There was a problem hiding this comment.
if we are not going to need this why keep it cmmented?
There was a problem hiding this comment.
I didn't remove it as I'm not sure if it's not supported by design or not supported just yet. From what I saw it should be fairly easy to modify to run it here as well.
There was a problem hiding this comment.
Please consider always removing as much code as possible (unnecessary code). what you can do here to be more specific is:
# run-transport-interop:
# currently this job is not implemented because of this and that. and it will be added later on after this and that
There was a problem hiding this comment.
that is remove all comments of old implementation right away. it is dead code
| echo Json.encode( | ||
| ResultJson( | ||
| handshakePlusOneRTTMillis: float(totalDelay.milliseconds), | ||
| pingRTTMilllis: float(pingDelay.milliseconds), | ||
| ) | ||
| ) | ||
| echo "latency:" | ||
| echo " handshake_plus_one_rtt: " & $float(totalDelay.milliseconds) | ||
| echo " ping_rtt: " & $float(pingDelay.milliseconds) | ||
| echo " unit: ms" |
There was a problem hiding this comment.
what type of output it is now?
if it is json we should use json, if it yaml. we should marshal some object with yaml
There was a problem hiding this comment.
it requires yaml output to stdout
There was a problem hiding this comment.
because looking in the code it is not clear what is going on. would be bettert ot have some object that is marshaled to stdout
echo Yaml.marshal(result)
Summary
Update Transport interop test script to be compatible with new Unified testing repo (see https://github.com/libp2p/unified-testing/blob/master/docs/write-a-transport-test-app.md).
Tested against a fork rlve/unified-testing#1 for now. Currently
python-v0.x(ws) is failing againstnim-v1.15.Warning
Currently the new repo doesn't support using their actions on the implementation side, that's why
run-transport-interopjob has to be disabled and the required check turned off.Affected Areas