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 web #1019

Merged
merged 7 commits into from
Feb 15, 2024
Merged

Add conformance tests for web #1019

merged 7 commits into from
Feb 15, 2024

Conversation

srikrsna-buf
Copy link
Member

Add conformance tests for web. Uses webdriverio package to spin up local browsers and run tests. Currently tests on Chrome, Firefox and Safari (if available).

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!

I wonder:

  • We only run 28 tests - is this correct, or are we inadvertently skipping important tests with our config?
  • Debugging browser tests - can we get access to the network inspector and logs?
  • Transports from @bufbuild/connect-web on Node.js - it supports fetch, and running the transports on Node is a valid use case that we cover with the make target testwebnode. How can we replace this coverage with conformance tests?

Comment on lines 44 to 47
case HTTPVersion.HTTP_VERSION_2:
break;
case HTTPVersion.HTTP_VERSION_3:
throw new Error("HTTP/2 and HTTP/3 are not supported");
Copy link
Member

Choose a reason for hiding this comment

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

There's a mismatch between error message and code path here.

I think we should be good to accept all three HTTP protocol versions here. Chrome and FF have supported h3 for a long time. https://caniuse.com/http3 Information about the state in Safari is muddy, but I see requests with h3 in the network inspector in 17.2.1, browsing to random sites.

Judging from the conformance runner output, we only run tests over plaintext HTTP/1.1. Is this because the runner configuration has supportsH2c: false?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is because we are still using rc1. If we upgrade to rc2 there are some Idempotency tests that fail for connect-node but they seem to be a problem of the runner, because using the latest runner we pass all the tests, so I refrained from upgrading until the next release.

Regarding HTTP/3, the latest runner actually even fails for HTTP/1, because it expects the client to use HTTP/1, even when the reference server supports HTTP/2. This I think should be changed, I am yet to report this.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, but please put a TODO at the H3 case.

@srikrsna-buf
Copy link
Member Author

  • We only run 28 tests - is this correct, or are we inadvertently skipping important tests with our config?

This is because we are still at rc1 and rc2 has a bug which is fixed in the latest runner. The latest runner runs 946 tests with the same config.

  • Debugging browser tests - can we get access to the network inspector and logs?

Yes added a section for running on a local browser, we can get access to both.

  • Transports from @bufbuild/connect-web on Node.js - it supports fetch, and running the transports on Node is a valid use case that we cover with the make target testwebnode. How can we replace this coverage with conformance tests?

Good point! I just included node as a browser option. Seemed straightforward. There's some more code we can share but I'll do that change in the conformance client PR for cloudflare.

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.

Thanks for adding @connectrpc/connect-web tests on Node! We're only running them in v20, but I think it's acceptable.

Regarding testwebconformancelocal, it isn't really possible to use the network inspector because the automated browser will close before you get a chance to open it.

It's crude and not ideal, but WDYT about changing the last line of runBrowser to:

  if (flags.headless === true) {
    await browser.deleteSession();
  } else {
    await browser.executeScript(`document.write("Tests done. You can inspect requests in the network explorer.")`, []);
  }

The conformance runner will time out, but it allows to inspect errors and the network in the browser's dev tools.

@srikrsna-buf
Copy link
Member Author

Great idea! Also added a flag to open the devtools on startup in local mode.

@srikrsna-buf srikrsna-buf merged commit fa7726b into main Feb 15, 2024
4 checks passed
@srikrsna-buf srikrsna-buf deleted the sk/conformance/web branch February 15, 2024 18:18
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.

3 participants