add go-libp2p v0.47.0 #788
Conversation
|
The "contract" has changed between the test framework and the test applications. It is fully documented in: write-a-transport-test-app.md If you don't want to change the test application in the go-libp2p repository, you can add a patch file in the images/ subfolders like I did for the images/rust/v0.56 transport interop test and then you add patchPath and patchFile to the implementation object in images.yaml |
Signed-off-by: Dave Grantham <[email protected]>
|
The app built and the tests ran but some are failing. ✗ Failed: 24
If you download the artifacts there's a |
|
Why did the contract change? |
|
To normalize it across all tests and to minimize dependencies. This makes it easier to write new tests (e.g. Kademlia) while also simplifying local development/debugging/performance testing. There are many improvements with this new test-plans framework. |
|
This is a cosmetic change of upper vs lower case env vars. All of the original interop tests used lower case env vars. If the argument is to normalize it, it should be normalized to lower case as that is what the existing code and spec defined. I'm not convinced that using upper case env vars makes anything easier. Can we please change it back? It seems painful to have to require every prior version of this test to include a patch to workaround a design issue that can still be fixed. |
|
The output format changed to YAML as well. Upper case is convention for environment variables. Has been for decades. It's already changed and you can just land the patch I wrote for you. |
|
The issue is not only with this version, but with all previous versions of this interop test.
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html
Emphasis added. These parameters are part of the application namespace. But even putting that aside, it would be nice if we didn't break existing applications for no good reason. Upper vs lower is cosmetic issue. JSON vs YAML is also a cosmetic issue (as an aside, JSON is valid YAML syntax, you could parse the json output as YAML just fine). |
|
I hear your arguments and I understand where you're coming from. Most developers recognize ENVIRONMENT_VARIABLES as such, despite the fine print in the POSIX spec. There's good arguments either way. As for the YAML/JSON output issue, we're minimizing dependencies and wanted to use YAML for the input config files because they allow us to add comments which JSON doesn't. Switching to YAML for everything eliminates an extra dependency. Plus there's a schema that aids in upstream reporting that isn't obvious in the transport tests but becomes apparent in the perf tests. I wrote you a patch to save you the work. The new framework supports local patches against test applications to decouple the testing from upstreaming changes in implementation repos. The rest of the test applications will be patched in short order since, as you point out, it's a trivial change. You might think this is an unnecessary and irrelevant change but the upper casing of environment variables brings this test framework in line with common convention and lowers the barrier to entry for developers looking at it for the first time. Also, the consistency across tests will pay off in the long term maintenance and expansion of this framework. Would you like me to submit the patch as a PR against go-libp2p? |
|
@MarcoPolo the next spec meeting is on February 10th: https://luma.com/yamxhbko That's where we include time to discuss this repo and the improvements for enforcing the specifications. Attending that is the best way to have influence over the test-plans roadmap. |
|
@dhuseby @MarcoPolo Take a look at #801 ( Updates / supersedes #788. Same bump-go changes but from my fork so the latest commit (fix: make go new transport spec compliant) is included. ) |
interop testing the next go-libp2p release