-
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
fix: util.parseArgs() missing node:process import #22405
Conversation
This looks good, but can you try enabling the See the notes in |
Removing the line? You mean adding it? It is already there. I wonder, if the test was already there, how come it has never failed? |
Oops! @kt3k corrected me below. |
@mmastrac @javihernant You can check it with the command:
The fixed code path (the case when I think we need to add our own test case in |
okay! Im going to try creating a custom test |
tests/unit_node/util_test.ts
Outdated
Deno.test({ | ||
name: "[util] parseArgs() with no args works", | ||
fn() { | ||
util.parseArgs(); |
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.
This looks causing a type error. I think we need to pass an empty object
util.parseArgs(); | |
util.parseArgs({}); |
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.
yep, that was the problem. I've fixed it in my fork. I've seen the CI test that fails is run with cargo test
. However, that takes a lot of time to run in my machine. What's the most appropriate way I should be running tests? I've used previously the ./target/debug/deno test --config tests/config/deno.json -A tests/unit_node/util_test.ts
command, which run only the tests of my PR. However, that didn't check for TS syntax error it seems.
fix parseArgs() not working due to missing import of node:process this commit fixes issue denoland#22363
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. Nice work!
fix parseArgs() not working due to missing import of node:process
this commit fixes issue #22363