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

Reorganize conformance tests #1079

Merged
merged 47 commits into from
May 16, 2024
Merged

Reorganize conformance tests #1079

merged 47 commits into from
May 16, 2024

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented May 8, 2024

This PR implements a reorganization of the conformance tests in the repo and how they are run.

The connect-conformance package previously ran all the conformance tests on its own. Now, the conformance tests have been moved to their respective packages and connect-conformance simply houses the common functionality used by all the packages to run conformance tests. Conformance tests have been relocated to:

  • connect-node - houses and runs both client and server conformance tests for Connect-Node.
  • connect-web - houses and runs client conformance tests for Connect-Web
  • connect-express - houses and runs server conformance tests using the connect-express plugin.
  • connect-fastify - houses and runs server conformance tests using the connect-fastify plugin.
  • connect-cloudflare (new) - houses and runs server conformance tests for connect-node on Cloudflare. This package exists solely for holding Cloudflare conformance tests (at least for now).

The remainder of connect-conformance should be no scripts, no tests running. Just a helper package.

Note that the Makefile (intentionally) no longer runs the Node-related conformance tests in all supported versions of Node. A follow-up PR will add a Github action matrix that runs these tests in various Node versions.

Makefile Show resolved Hide resolved
cd packages/connect-conformance && PATH="$(abspath $(BIN)):$(PATH)" node18 ./bin/connectconformance --mode client --conf conformance-node.yaml -v ./bin/conformancenodeclient
cd packages/connect-conformance && PATH="$(abspath $(BIN)):$(PATH)" node20 ./bin/connectconformance --mode client --conf conformance-node.yaml -v ./bin/conformancenodeclient
cd packages/connect-conformance && PATH="$(abspath $(BIN)):$(PATH)" node21 ./bin/connectconformance --mode client --conf conformance-node.yaml -v ./bin/conformancenodeclient
testnodeconformance: $(BUILD)/connect-conformance $(BUILD)/connect-node $(BUILD)/connect-fastify $(BUILD)/connect-express
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that we are intentionally not running these in all Node versions. A future PR will use a Github action matrix to run against multiple Node versions.

Copy link
Member

Choose a reason for hiding this comment

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

We should also (as follow-ups):

  • find a solution for test coverage for createCallbackClient,
  • solve the connectconformance issue described in Reorganize conformance tests #1079 (review),
  • move tests we want to keep from connect-node-test to connect-node (similar for connect-web-test) and delete these packages,
  • remove all remaining bits of the old crosstests

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, sounds good. Created #1080 to track all these (plus the version-in-two-places issue).

@smaye81 smaye81 changed the title Add additional conformance tests for Fastify and Express Reorganize conformance tests May 14, 2024
@smaye81 smaye81 marked this pull request as ready for review May 14, 2024 20:19
@smaye81
Copy link
Member Author

smaye81 commented May 15, 2024

@srikrsna-buf we can land #1066 now whenever and I can merge main into this.

packages/connect-web/package.json Outdated Show resolved Hide resolved
packages/connect-web/package.json Outdated Show resolved Hide resolved
packages/connect-conformance/src/web/transport.ts Outdated Show resolved Hide resolved
packages/connect-conformance/package.json Show resolved Hide resolved
packages/connect-conformance/package.json Outdated Show resolved Hide resolved
@smaye81 smaye81 requested a review from timostamm May 15, 2024 15:04
@smaye81 smaye81 requested a review from timostamm May 15, 2024 18:11
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.

Nice, this is taking shape.

I left some comments regarding documentation and code style below.

Regarding the test runner connectconformance:

  1. Currently, the version is maintained in two places, it would be better to have just a single one.
  2. The shim downloads the release on the fly and caches it. This won't work for parallel runs, and we'll use parallel runs with turborepo.

We've solved both problems in protobuf-es here. We should solve them here as well, but I don't think it's necessary to do it in this PR.

packages/connect-cloudflare/README.md Show resolved Hide resolved
packages/connect-web/conformance/client.ts Outdated Show resolved Hide resolved
@smaye81 smaye81 mentioned this pull request May 16, 2024
5 tasks
Co-authored-by: Timo Stamm <ts@timostamm.de>
@smaye81 smaye81 merged commit e580470 into main May 16, 2024
5 checks passed
@smaye81 smaye81 deleted the sayers/conformance_reorg branch May 16, 2024 18:17
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.

None yet

3 participants