fix: add js tests to this repo #365
Conversation
Trying to debug these tests in CI is incredibly difficult and since they depend on older versions of previously published modules, if something breaks it's very hard to fix it. Instead follow the pattern in the perf tests and include the full test implementations here.
👍. This is what I originally did here. The other impls like Go still have the the interop code in the main repo, and then copy it to their impl repo when Related: #364 What did you change to fix it? My hunch is that some timing is off between when the local redis proxy starts and when the test code makes a request for it. This line makes me think that: |
| } | ||
|
|
||
| const proxyServer = http.createServer(requestListener) | ||
| proxyServer.listen(0) |
There was a problem hiding this comment.
This is what fixed it. Previously this was:
await new Promise(resolve => { proxyServer.listen(0, 'localhost', () => { resolve() }) })
The reason this broke it is kind of funny. For whatever reason this now listens on only the IPv6 loopback address ::1, but when we do the fetch:
fetch(`http://localhost:...
it resolves to the ipv4 loopback address at 127.0.0.1.
Apparently in this environment just doing .listen(0) listens on both ::1 and 127.0.0.1 (unless IPv6 is disabled and you only listen on 127.0.0.1).
There was a problem hiding this comment.
Yeah, it's generally better to listen without specifying a host, then it'll bind to all available addresses/interfaces, then the host can resolve "localhost" to whatever it wants and we can have a quiet life 😄
|
Can we either:
I'd lean for option 1 since #359 should be good to go now after I fixed it with an ugly hack. |
|
My preference would still be to use released artefacts to run these tests, if nothing else because installing all the dev tools and building things takes time which will only get worse the more releases there are, but it seems that the runtime environment will change over time (ex, these tests used to pass, now they don't because of the IP4/IP6 thing identified above) so this is probably the pragmatic thing to do. I see #359 got merged, I didn't realise we were going to add every minor release too - TBH I'm not sure of the utility of testing The transport and identify/ping protocol modules do all the heavy lifting here so are the ones that would be likely to introduce bugs that break things but That is to say, we could be on Instead we could just have the latest For total certainty we'd need have to have versions here for every monorepo publish but that would lead to many jobs here and ever increasing build times.
|
Aligns interop test implementation with libp2p/test-plans#365
Aligns interop test implementation with libp2p/test-plans#365
I'll defer to your opinion on how to do the js-libp2p implementation here. I'll just mention that the time for installing/building is probably a non-issue since we cache the builds anyways. Go and Rust (and others) only build the test once. I would suggest considering following how Go/Rust implement the test in the main repo and copy it during a build. That gives you the benefit of having the test defined alongside the implementation and being able to debug things like this, or, the more likely failure, an incompatibility.
It's only testing 1.5 and 1.6 (the last two versions). The other versions are vestiges that I haven't remove yet.
That sounds fine by me. As long as a given commit is reproducible I'm happy. |
The test in js-libp2p main should be compatible with the currently released major (v1 at the time of writing), so on the face of it, it makes sense to copy over during the build. But if we release v2, it may become incompatible with v1, so we should copy the v1 test from the latest We can always revisit this later if the situation changes. |
Aligns interop test implementation with libp2p/test-plans#365
Trying to debug these tests in CI is incredibly difficult and since they depend on older versions of previously published modules, if something breaks due to a bug in released code it's very hard to fix it.
Instead follow the pattern in the perf tests and include the full test implementations here.
The changes here seem to make the js-libp2p transport interop CI job much happier!