Tweak error codes according to the conformance suite #1063
Conversation
| @@ -22,7 +22,7 @@ import { TestService } from "../gen/connectrpc/conformance/v1/test_connect.js"; | |||
| import { describeTransports } from "../helpers/conformanceserver.js"; | |||
| import { Empty } from "@bufbuild/protobuf"; | |||
|
|
|||
| describe("unimplemented_server_streaming_method", function () { | |||
| xdescribe("unimplemented_server_streaming_method", function () { | |||
There was a problem hiding this comment.
Is this meant to be skipped (along with the callback client one above?)
There was a problem hiding this comment.
Yes, this fails for both
Then we'd end up not having tests for that, or we can update the conformance version first by adding a bunch of know failing, and then add the fixes removing the know failing cases. It didn't seem worth it because the update is just a version bump and deleting some files, and all the fixes are status code related. |
|
Seems fine to me, but let's wait to see what @timostamm thinks. |
There was a problem hiding this comment.
If we cut a release, this PR shows up in the changelog, and the title should tell users what we are changing.
The PR title doesn't give any indication that we're changing error codes, and it's not clear what exactly changes from looking at the diff.
We need better communication. For reference, see connectrpc/connect-go#706.
I suggest to change the PR title to be about the change to the code mappings, and only mention the conformance update in the description. You can link to the connectrpc.com PR, same as the connect-go change did. If there are other changes beyond the status code changes in this PR, please list them as well.
| "node-forge": "^1.3.1", | ||
| "asn1js": "^3.0.5", |
srikrsna-buf
changed the title
Tweak error codes according to the conformance suite. Users should not notice any difference as most of the tweaks are related to errors caused due to mis-implementation of the protocol.
Also update conformance runner to
v1.0.2. This lets us shednode-forgeandasn1jswhich we relied to generate a TLS cert.