-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat(cli): evaluate code snippets in JSDoc and markdown #25220
feat(cli): evaluate code snippets in JSDoc and markdown #25220
Conversation
@magurotuna this looks great, can we use it to test our own declarations files (eg. |
I did (against deno_std as well), and what turned out was some fail because they are not self-contained and make some assumption on the testing environment that is not always the case, or some even hung - which made me feel we need to preserve the existing type-checking only mode in another command 😄 Here's the result of running This says that the test case starting from line 2781 hangs - and actually this test case is waiting for something from stdin, which obviously doesn't occur, thus resulting in unfinished: deno/cli/tsc/dts/lib.deno.ns.d.ts Lines 2781 to 2787 in c29e5b9
And one of the failing test cases is this, where it tries to open deno/cli/tsc/dts/lib.deno.ns.d.ts Lines 2725 to 2728 in c29e5b9
So I think we can conclude that the new doc test feature is working as we expect, although many of the existing example code snippets in JSDoc need to be updated. |
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.
Looks good! Just a question and a few minor things.
cli/tools/check.rs
Outdated
use MediaType::*; | ||
matches!( | ||
MediaType::from_specifier(specifier), | ||
TypeScript | JavaScript | Tsx | Jsx | Mts | Mjs | Cts | Cjs |
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.
Why do we do this filtering here, but not in the non-doc case?
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.
The reason is to preserve the existing deno test --doc
behavior. For instance, using v1.46.3 of Deno, running deno test --doc cli/tsc/dts/lib.deno.ns.d.ts
succeeds:
$ deno test --doc cli/tsc/dts/lib.deno.ns.d.ts
Check file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns$d$ts$22-28.ts
Check file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns$d$ts$64-69.ts
Check file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns$d$ts$75-79.ts
... omitted ...
Check file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns$d$ts$6180-6186.ts
Check file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns$d$ts$6198-6206.ts
ok | 0 passed | 0 failed (0ms)
But if we don't do the filtering here, ./target/debug/deno check --doc cli/tsc/dts/lib.deno.ns.d.ts
does fail.
$ ./target/debug/deno check --doc cli/tsc/dts/lib.deno.ns.d.ts
Check file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns.d.ts
Check file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns$d$ts$22-28.ts
Check file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns$d$ts$64-69.ts
... omitted ...
Check file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns$d$ts$6180-6186.ts
Check file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns$d$ts$6198-6206.ts
error: TS2304 [ERROR]: Cannot find name 'TextDecoder'.
const decoder = new TextDecoder("utf-8");
~~~~~~~~~~~
at file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns$d$ts$1007-1032.ts:16:25
TS2304 [ERROR]: Cannot find name 'TextDecoder'.
const decoder = new TextDecoder("utf-8");
~~~~~~~~~~~
at file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns$d$ts$1045-1064.ts:14:25
... omitted ...
TS2304 [ERROR]: Cannot find name 'BufferSource'.
destination: BufferSource,
~~~~~~~~~~~~
at file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns.d.ts:5877:20
TS2304 [ERROR]: Cannot find name 'URL'.
filename: string | URL,
~~~
at file:///Users/yusuke/Repo/github.com/magurotuna/deno/cli/tsc/dts/lib.deno.ns.d.ts:6087:24
Found 269 errors.
With the filtering, we can get the same result as the current deno test --doc
.
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.
Why don't we want to type check .d.ts
files or type check their code snippets in this case?
Also, getting the media type from the specifier alone is not accurate for some remote specifiers without an extension.
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.
Ah OK I get it now - Fixed fbc7b8e
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.
Oh I now remember why I added the filter - we have a test case to confirm that deno check --doc cli/tsc/dts/lib.deno.ns.d.ts
(which previously was deno test --doc
) passes:
deno/tests/integration/check_tests.rs
Lines 189 to 210 in 50d87d3
#[test] | |
fn typecheck_declarations_ns() { | |
let context = TestContextBuilder::for_jsr().build(); | |
let args = vec![ | |
"check".to_string(), | |
"--doc".to_string(), | |
util::root_path() | |
.join("cli/tsc/dts/lib.deno.ns.d.ts") | |
.to_string_lossy() | |
.into_owned(), | |
]; | |
let output = context | |
.new_command() | |
.args_vec(args) | |
.envs(util::env_vars_for_jsr_tests()) | |
.split_output() | |
.run(); | |
println!("stdout: {}", output.stdout()); | |
println!("stderr: {}", output.stderr()); | |
output.assert_exit_code(0); | |
} |
The previous command deno test --doc cli/tsc.dts/lib.deno.ns.d.ts
was successful because it was checking only the snippets in the doc comments, and did not type check the whole file. But since now we move the ability to perform type-check against doc comments into check --doc
command, it will run type-check against the declaration part of lib.deno.ns.d.ts, resulting in a bunch of type errors. (see the CI error log at https://github.com/denoland/deno/actions/runs/10849599526/job/30109161558?pr=25220#step:43:5705)
Do you have any idea to resolve this issue? My quick idea is that to make check --doc
only type-check on code snippets in doc comments, but not sure if this really is a good interface for users.
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.
After some thoughts, I decided to add --doc-only
option to the check
subcommand, which allows us to perform the type checking only on code snippets in JSDoc or markdown. The rationale of having this new flag separately is deno check
command doesn't accept markdown files, but we want to type check on code snippets in markdown - which --doc-only
accomplishes.
@@ -0,0 +1,5 @@ | |||
{ | |||
"args": "check --doc --config ../../../config/deno.json mod.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.
Can you change this so that the spec test is self contained and doesn't traverse up ancestor directories?
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.
I referred to other existing spec tests like:
"args": "run -A --unstable-fs --config ../../../config/deno.json main.js", |
In any case I don't think we can get rid of relative path completely because we want to import @std modules from the git submodule which is located at <repo_root>/tests/util/std
. Do you have any good idea to make this self-contained?
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's a good catch. We need to find a better solution here
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.
We could have deno.json
in each test directory that contains importMap: "../../../../import_map.json"
to utilize the import map located at the repo root, but this still has relative path and thus not self-contained. Any other ideas?
Two blocking concerns, IMO:
|
Yes it does. I added another test case to illustrate it 13235e2. Let's clarify this in the document later.
The existing directive |
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.
LGTM, this is a great first pass and we can keep iterating on this one. @magurotuna could you please update the docs page at https://docs.deno.com/runtime/fundamentals/testing/?
Yes! Will do |
) We landed "true" doc testing in denoland/deno#25220. With this change, `deno test --doc` will actually _run_ code snippets written in JSDoc or markdown files, as well as type checking. This commit updates the corresponding doc pages to follow this change.
This commit lets
deno test --doc
command actually evaluate code snippets in JSDoc and markdown files.How it works
Deno.test(...)
We apply some magic at the step 2 - let's say we have the following file named
mod.ts
as an input:This is virtually transformed into:
Note that a new import statement is inserted here to make
add
function available. In a nutshell, all items exported frommod.ts
become available in the generated pseudo file with this automatic import insertion.The intention behind this design is that, from library user's standpoint, it should be very obvious that this
add
function is what this example code is attached to. Also, if there is an explicit import statement likeimport { add } from "./mod.ts"
, this import path./mod.ts
is not helpful for doc readers because they will need to import it in a different way.The automatic import insertion has some edge cases, in particular where there is a local variable in a snippet with the same name as one of the exported items. This case is addressed by employing swc's scope analysis (see test cases for more details).
"type-checking only" mode stays around
This change will likely impact a lot of existing doc tests in the ecosystem because some doc tests rely on the fact that they are not evaluated - some cause side effects if executed, some throw errors at runtime although they do pass the type check, etc.
To help those tests gradually transition to the ones runnable with the new
deno test --doc
, we will keep providing the ability to run type-checking only viadeno check --doc
. Additionally there is a--doc-only
option added to thecheck
subcommand too, which is useful when you want to type-check on code snippets in markdown files, as normaldeno check
command doesn't accept markdown.Demo
deno_test_doc_demo.mp4
Closes #4716