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

Update conformance testee for @connectrpc/connect-web #1183

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

timostamm
Copy link
Member

@timostamm timostamm commented Aug 22, 2024

Several small changes, see below.

The Firefox known-failing cases are a caused by a bug in the Firefox webdriver: AbortSignal.abort().reason instanceof Error is false when run through WebdriverIO.Browser.executeScript.
We shouldn't need to ignore this. I plan to fix it in a follow-up.

The callback client known-failing cases happen because we swallow the AbortError for both client-side cancel and server-side cancel. It makes sense for the callback-API for client-side cancel, but it would be good to not do this for the server-side.

Signed-off-by: Timo Stamm <ts@timostamm.de>
@timostamm timostamm force-pushed the tstamm/update-connect-web-conformance branch from f7a442d to 8c28884 Compare August 22, 2024 16:13
Comment on lines +60 to +61
node:
runs-on: ubuntu-22.04
Copy link
Member Author

Choose a reason for hiding this comment

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

We're running conformance tests for @connectrpc/connect-web on Node.js now. They run quick enough, seems worth it.

async function extractBin(path: string) {
if (path.endsWith(".zip")) {
const unzipped = unzipSync(readFileSync(path), {
async function extractBin(archivePath: string, binPath: string) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Running the connectconformance command in parallel used to fail without an error message, because this script tried to write to the file that was already running.


.PHONY: testwebconformancelocal
Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't work as documented. Since the npm script runs two commands with &&, the --browser flag was only appended to the second command. The replacement is explained in packages/connect-conformance/README.md

Comment on lines +51 to +53
value: new ClientErrorResult({
message: `Failed to run test case: ${String(err)}`,
}),
Copy link
Member Author

Choose a reason for hiding this comment

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

Wrapping the error isn't helpful here, this keeps more info.

Comment on lines +134 to +136
`const p = document.createElement("p");
p.innerText = "Tests done. You can inspect requests in the network explorer."
document.body.append(p);`,
Copy link
Member Author

Choose a reason for hiding this comment

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

document.write raises an error in Firefox.

@@ -34,11 +33,13 @@ const { values: flags } = parseArgs({
options: {
browser: { type: "string", default: "chrome" },
headless: { type: "boolean" },
openBrowser: { type: "boolean" },
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of opting in to run headless with --headless, you now opt in to run with UI with --openBrowser. This allows us to re-use the npm scripts. It's explained in packages/connect-conformance/README.md

"conformance:client:safari": "connectconformance --mode client --conf ./conformance/conformance-web.yaml -v -- ./conformance/client.ts --browser safari --headless && connectconformance --mode client --conf ./conformance/conformance-web.yaml -v --known-failing @./conformance/known-failing-callback-client.txt -- ./conformance/client.ts --browser safari --headless --useCallbackClient",
"conformance:client:node": "connectconformance --mode client --conf ./conformance/conformance-web-node.yaml -v -- ./conformance/client.ts --browser node && connectconformance --mode client --conf ./conformance/conformance-web-node.yaml -v --known-failing @./conformance/known-failing-callback-client.txt -- ./conformance/client.ts --browser node --useCallbackClient",
"conformance:client:browser": "connectconformance --mode client --conf ./conformance/conformance-web.yaml -v -- ./conformance/client.ts && connectconformance --mode client --conf ./conformance/conformance-web.yaml -v --known-failing @./conformance/known-failing-callback-client.txt -- ./conformance/client.ts --useCallbackClient",
"conformance:client:chrome:promise": "connectconformance --mode client --conf ./conformance/conformance-web.yaml -v -- ./conformance/client.ts --browser chrome",
Copy link
Member Author

Choose a reason for hiding this comment

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

That's an awkward amount of scripts, but it's useful to be able to run just one suite.

@timostamm timostamm requested a review from smaye81 August 22, 2024 16:41
@timostamm timostamm merged commit f95d15b into main Aug 26, 2024
21 checks passed
@timostamm timostamm deleted the tstamm/update-connect-web-conformance branch August 26, 2024 06:46
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.

2 participants