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

--inspect can no longer be used as a flag through gulp #42701

Merged
merged 3 commits into from
Feb 13, 2021

Conversation

andrewbranch
Copy link
Member

gulp-cli removes --inspect and --inspect-brk from process.argv and passes it straight to the Node process that’s running gulp, which is not the same Node process that runs mocha for us, so our test runner never sees --inspect as a flag. Using -i also wasn’t working, as that launched node with --inspect-brk=true, so I fixed that, and updated docs.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Feb 8, 2021
@weswigham
Copy link
Member

...wait, how did this change? The last publish of gulp itself is 2 years ago, and gulp-cli 8 months ago. How long has this been broken for... (I mean, I guess I invoke mocha directly in my daily workflow, so I'd not have noticed)? o.O

@andrewbranch
Copy link
Member Author

It’s been broken for 8 months.

@andrewbranch
Copy link
Member Author

So, I tested --debug and found that doesn’t get stripped, but now seeing how this works, it looks like if you were using a version of node where you could use --debug, that would get stripped, since the recognized flags are determined dynamically. We can probably just delete that option, or alias it to --inspect.

@weswigham
Copy link
Member

Maybe we should just rename it to a 3rd name that doesn't conflict? --break?

@andrewbranch
Copy link
Member Author

--debug has been removed, --break has been added.

scripts/build/options.js Outdated Show resolved Hide resolved
@@ -453,8 +453,7 @@ task("runtests").flags = {
"-t --tests=<regex>": "Pattern for tests to run.",
" --failed": "Runs tests listed in '.failed-tests'.",
"-r --reporter=<reporter>": "The mocha reporter to use.",
"-d --debug": "Runs tests in debug mode (NodeJS 6 and earlier)",
"-i --inspect": "Runs tests in inspector mode (NodeJS 8 and later)",
"-i --break": "Runs tests in inspector mode (NodeJS 8 and later)",
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want -i to be the shorthand for `--break?

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's backward compatible

Copy link
Member

Choose a reason for hiding this comment

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

I'd sorta doubt anyone is taking a hard automated dependency on it, so I wouldn't worry about that too much. 😄

Either way I'll leave it up to you

Copy link
Member Author

Choose a reason for hiding this comment

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

-b is already taken by --browser despite not being documented in the Gulpfile, so I’m not sure what else to call it.

Copy link
Member

Choose a reason for hiding this comment

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

Well...who even runs that one? Anyway, it's not a big deal.

@andrewbranch andrewbranch merged commit d3055f0 into microsoft:master Feb 13, 2021
@andrewbranch andrewbranch deleted the bug/debug branch February 13, 2021 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants