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

Add windowsVerbatimArguments for node child_process #20694

Merged
1 commit merged into from Nov 6, 2017

Conversation

andrewstucki
Copy link
Contributor

To go along with nodejs/node#16299.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

@dt-bot
Copy link
Member

dt-bot commented Oct 18, 2017

types/node/index.d.ts

to authors (@DefinitelyTyped/DefinitelyTyped @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @DeividasBakanas @kjin @alvis Microsoft TypeScript (account can't be detected)). Could you review this PR?
👍 or 👎?

@typescript-bot typescript-bot added the Popular package This PR affects a popular package (as counted by NPM download counts). label Oct 18, 2017
@typescript-bot
Copy link
Contributor

@andrewstucki Please fix the failures indicated in the Travis CI log.

@andrewstucki
Copy link
Contributor Author

Closing because of PR build timeouts

@andrewstucki
Copy link
Contributor Author

Re-opening to re-run failed test because of PR build timeouts

@andrewstucki
Copy link
Contributor Author

I don't think that this is going to pass in travis due to the sheer number of type definitions that are affected by the definitions in types/node. The build is timing out while running the tests because it's going through pretty much every single test possible.

@Flarna
Copy link
Contributor

Flarna commented Oct 19, 2017

If I understand the node source code correct then exeFile allows also to specify windowsVerbatimArguments.

@andrewstucki
Copy link
Contributor Author

Looks like @Flarna is right. execFile doesn't override shell, so it should still be valid to pass in windowsVerbatimArguments. Updated.

@andrewstucki
Copy link
Contributor Author

so... wondering what to do with this given that the build can't possibly pass due to the fact that it requires testing 1211 packages and exceeds Travis's time limit for jobs every time it runs?

@smac89
Copy link
Contributor

smac89 commented Oct 26, 2017

@andrewstucki I believe you can do something like:

Forking the repo, create the travis hook on your account, run the tests there and try to do another commit after that. Travis will be able to skip running the long tests if you have already run it through travis once.

@andrewstucki
Copy link
Contributor Author

@smac89 I enabled travis on my branch and am going to see if it can pass there, but seeing as there appears to be a global travis timeout of 50 mins for jobs and the CI build above was canceled at 49:57, I'm kind of doubtful that I can get the tests to pass there.

@andrewstucki
Copy link
Contributor Author

See #20308

@RyanCavanaugh
Copy link
Member

@andrewstucki Please fix the failures indicated in the Travis CI log.

@andrewstucki
Copy link
Contributor Author

@RyanCavanaugh as mentioned multiple times, this relates to #20308 -- the build is timing out because of the fact that this is a change to one of the definitions for node which is a dependency of pretty much every other definition in this project. It's not possible to fix because Travis times out before it can finish testing every single type definition.

@andrewstucki
Copy link
Contributor Author

@andy-ms noticed you resolved and merged #20185

Seeing as this is related because of the Travis timeouts and has sat here untouched for 2 weeks, think you could take a look? Otherwise I think I'm just going to close this PR soon as it looks like it's getting no attention.

@ghost ghost merged commit ce7836d into DefinitelyTyped:master Nov 6, 2017
KSXGitHub pushed a commit to KSXGitHub/DefinitelyTyped that referenced this pull request May 12, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Popular package This PR affects a popular package (as counted by NPM download counts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants