Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add conformance tests for connect-node #886

Merged
merged 18 commits into from
Dec 1, 2023
Merged

Conversation

srikrsna-buf
Copy link
Member

@srikrsna-buf srikrsna-buf commented Oct 27, 2023

Add connect-node-conformance package with server implementation. Based on the reference server implementation.

Tested with the following conformance config:

features:
  versions:
    - HTTP_VERSION_1
    - HTTP_VERSION_2
  protocols:
    - PROTOCOL_CONNECT
    - PROTOCOL_GRPC
    - PROTOCOL_GRPC_WEB
  codecs:
    - CODEC_PROTO
    - CODEC_JSON
  compressions:
    - COMPRESSION_IDENTITY
    - COMPRESSION_GZIP
    - COMPRESSION_BR
  supportsTls: true
  supportsTlsClientCerts: true
  supportsConnectGet: true
  supportsHalfDuplexBidiOverHttp1: true
  requiresConnectVersionHeader: false
  supportsMessageReceiveLimit: true

@srikrsna-buf srikrsna-buf marked this pull request as ready for review October 27, 2023 15:28
packages/connect-node-conformance/src/protocol.ts Outdated Show resolved Hide resolved
packages/connect-node-conformance/src/routes.ts Outdated Show resolved Hide resolved
packages/connect-node-conformance/src/server.ts Outdated Show resolved Hide resolved
packages/connect-node-conformance/src/server.ts Outdated Show resolved Hide resolved
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

I did not take a close look at the server implementation, but this looks solid 👍

Left one comment about error detail debug values.

The readme for this new package needs more details, and the test run need to be wired up in make. I guess it only makes sense to do that after conformance v1 has been released.

The old crosstests need to be removed from this repo. I guess this is only feasible after #890 (we'll also want to run a Node fetch client, a browser fetch client, and at least retain the browserstack tests).

packages/connect-node-conformance/src/protocol.ts Outdated Show resolved Hide resolved
packages/connect-node-conformance/src/protocol.ts Outdated Show resolved Hide resolved
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Still looking solid to me 🙂 Would be great to have one of the conformance authors take a look, though.

Doesn't seem great to merge before conformance is released. What's your take?

packages/connect-node-conformance/src/protocol.ts Outdated Show resolved Hide resolved
@srikrsna-buf
Copy link
Member Author

srikrsna-buf commented Nov 22, 2023

Still looking solid to me 🙂 Would be great to have one of the conformance authors take a look, though.

Doesn't seem great to merge before conformance is released. What's your take?

Agree on conformance authors taking a look, about merging before v1 we are only merging to a branch so that I can continue to work on other environments

@srikrsna-buf srikrsna-buf changed the base branch from conformance-v2 to main November 29, 2023 14:11
@srikrsna-buf srikrsna-buf changed the title Add connect-node-conformance with server implementation Add conformance tests for connect-node Nov 30, 2023
@srikrsna-buf
Copy link
Member Author

@timostamm @smaye81 Added the test to CI. This is now good to go

Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Would be nice to remove the dep on fflate as noted in #886 (comment).

LGTM from me, but let's have @smaye81 take a look.

packages/connect-node-conformance/package.json Outdated Show resolved Hide resolved
packages/connect-node-conformance/src/routes.ts Outdated Show resolved Hide resolved
packages/connect-node-conformance/src/routes.ts Outdated Show resolved Hide resolved
packages/connect-node-conformance/src/routes.ts Outdated Show resolved Hide resolved
@smaye81
Copy link
Member

smaye81 commented Nov 30, 2023

Sorry for the delay @srikrsna-buf. Left some comments.

@@ -0,0 +1,21 @@
features:
Copy link
Member

Choose a reason for hiding this comment

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

Minor nits that can help clean this up:

  • You can omit protocols and codecs since all options are supported (if absent, all are assumed)
  • You can also omit supportsTls, supportsConnectGet, requiresConnectVersionHeader, and supportsMessageReceiveLimit since you have them listed as the defaults and if absent, the defaults are assumed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did think of omitting the defaults but it just felt easier to parse and see what is included just by looking at the config. I'll omit them in a separate PR.

@srikrsna-buf srikrsna-buf merged commit 708fefe into main Dec 1, 2023
4 checks passed
@srikrsna-buf srikrsna-buf deleted the conformance/server branch December 1, 2023 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants