-
Notifications
You must be signed in to change notification settings - Fork 410
Create typescript tests for apis #2480
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AdityaAtulTewari.
build/wd_test.bzl
Outdated
data = data + [src, "//src/workerd/server:workerd"] | ||
data = data + [src, "//src/workerd/server:workerd"] + srcs | ||
|
||
ts_srcs = [src for src in srcs if src.endswith(".ts")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you need the new srcs
argument at all? Why not just filter data
for arguments that end in *.ts
? It seems simpler to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Honestly it was cleaner to help me debug at first, I can collapse them.
src/workerd/api/http-test-ts.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the failure look like if there is a type error? Would it be possible to add a test_ts_testing
that forces a type error and asserts that it looks correct? Or did you manually verify this?
It looks to me like type errors generate build errors rather than test failures. Ideally we should probably use tsc --noCheck
at build time and run tsc --noEmit
as part of the test. But annoyingly, it looks like the --noCheck
CLI option is only going to be released as part of typescript v5.6:
microsoft/TypeScript#58839
So maybe we can add this as a TODO for when typescript 5.6 comes out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- That makes sense I can add that todo
8ad05a6
to
0d67399
Compare
0d67399
to
05a6ecf
Compare
No description provided.