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

test: move common.ArrayStream to separate module #22447

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 21, 2018

In a continuing effort to de-monolithize require('../common'),
move common.ArrayStream out to a separate module that is
imported only when it is needed.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 21, 2018
@@ -413,6 +413,19 @@ Platform normalizes the `pwd` command.

Synchronous version of `spawnPwd`.

## ArrayStream Module
Copy link
Member

Choose a reason for hiding this comment

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

Does arrayStream needs to be removed from line 41?

Copy link
Member

Choose a reason for hiding this comment

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

Since that seemed self-evident, I went ahead and did it. Separate commit so if I'm wrong, it can be rebased out easily.

@jasnell
Copy link
Member Author

jasnell commented Aug 22, 2018

In a continuing effort to de-monolithize `require('../common')`,
move `common.ArrayStream` out to a separate module that is
imported only when it is needed.
@jasnell
Copy link
Member Author

jasnell commented Aug 22, 2018

CI failed with infrastructure and lint issues.. trying yet again: https://ci.nodejs.org/job/node-test-pull-request/16682/

@jasnell
Copy link
Member Author

jasnell commented Aug 22, 2018

@nodejs/build ... linuxone build bot is failing consistently again.

@refack
Copy link
Contributor

refack commented Aug 22, 2018

@nodejs/build ... linuxone build bot is failing consistently again.

Fixed - https://ci.nodejs.org/job/node-test-commit-linuxone/4284/

@Trott
Copy link
Member

Trott commented Aug 23, 2018

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 23, 2018
jasnell added a commit that referenced this pull request Aug 23, 2018
In a continuing effort to de-monolithize `require('../common')`,
move `common.ArrayStream` out to a separate module that is
imported only when it is needed.

PR-URL: #22447
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Aug 23, 2018

Landed in fa543c0

@jasnell jasnell closed this Aug 23, 2018
targos pushed a commit that referenced this pull request Aug 24, 2018
In a continuing effort to de-monolithize `require('../common')`,
move `common.ArrayStream` out to a separate module that is
imported only when it is needed.

PR-URL: #22447
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
In a continuing effort to de-monolithize `require('../common')`,
move `common.ArrayStream` out to a separate module that is
imported only when it is needed.

PR-URL: #22447
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants