Add `.all` property with interleaved stdout and stderr #171

tomsotte · 2019-01-20T19:18:03Z

Added test cases for result.all intermixed output.

Added the fixture noop-132 which should output the sequence 123 (where 13 comes from stdout and 3 comes from stderr). Due to the unpredictable nature of the stdout/stderr it outputs 132 as the most probable case.
I've tried, as listed on noop-132, a different number of technique to make it predictable.

On execa.sync, as it's using spawnSync internally, I couldn't get the stream and merge them.

As such the output of result.all is just a concatenation of stdout + stderr.
This makes the result.all in different use case (ex. async vs. sync) unreliable for string comparison, but at least useful for logging or checking on the output.

Edit: Removed result.all on execa.sync, as it's just a concatenated stdout and stderr output, not very useful. Added execa.all() similar to execa.stdout(). Added all in errors (only for async). Added documentation.

Closes #1

Added test cases for result.all intermixed output. Added fixtures/noop-132 which should output the sequence '123' but due to the unpredictable nature of the stdout/stderr it outputs '132' as the most common cases I've tried, as listed on noop-132, a different number of technique to make it predictable. On execa.sync, as it's using spawnSync internally, I couldn't get the stream and merge them. As such the output of result.all is just a concatenation of stdout + stderr. This makes the result.all in different use case (ex. async vs. sync) unreliable for string comparison, but at least useful for logging or checking on the output.

sindresorhus · 2019-03-06T10:12:06Z

@ehmicky Would you be able to help review this?

index.js

ehmicky · 2019-03-06T11:06:08Z

fixtures/noop-132

@@ -0,0 +1,14 @@
+#!/usr/bin/env node


This does not test interleaving because stderr is not in-between two stdout calls.

It was my mistake, I should have left it in-between and let the test fail. As I said in the comments I was testing various way to implement the test to be correct but I couldn't find one for all (more on this on the next conversation).

Also I think it might be a good idea to do a more complex interleaving like stdout > stderr > stderr > stdout > stderr > stdout

I don't think it needs to be more complex, if it works just once we should expect to interleave the rest of the outputs. Otherwise we know it's just concatenating the output strings.
From what I understand by the tests I'm doing is that we cannot guarantee the order of the interleaving.

What do you think @sindresorhus?

fixtures/noop-132

ehmicky · 2019-03-06T11:08:23Z

Done! Also I think documentation would need to be added in the README.

… nothing - execa.sync() cannot provide a true interleaved `all` stream because of the implementation of child_process.spawnSync, which does not expose `stdout` and `stderr` streams until termination. We could only concatenate the streams in the end. - Does not create the mixed stream if both streams do not exist.

…ync()

The delay 6000ms is approximate, it may not work on every machine.

sindresorhus · 2019-03-07T14:28:30Z

Can you fix the merge conflict?

tomsotte · 2019-03-08T10:19:38Z

Ok, now I fixed it. I thought it was enough to fix them locally and then commit to remote.

…132 (1000ms) Testing with test.serial and a delay of only 1000ms seems to pass multiple test runs consistently. Let's see if it passes on CI testing as well.

tomsotte · 2019-03-09T13:13:03Z

Shall I add an execa.all() in the same vein as execa.stdout()?

test.js

ehmicky · 2019-03-09T13:29:23Z

@tomsotte this seems like a good idea :)

Thanks for your work on this, it's a very useful feature!

tomsotte · 2019-03-09T15:36:51Z

Should errors have the all output? At the time I did not added as I thought stdout and stderr would have been enough, but it might be helpful or at least it could be there for consistency with the normal result. What do you think?

ehmicky · 2019-03-09T15:39:20Z

Yes nice catch 👍

index.js

fixtures/noop-132

index.js

readme.md

test.js

sindresorhus · 2019-03-10T17:04:21Z

Thanks for contributing, @tomsotte :)

sindresorhus · 2019-03-10T17:05:04Z

@tomsotte You need to submit the PR URL to IssueHunt to claim the bounty.

tomsotte · 2019-03-10T17:16:25Z

Thanks to you both @sindresorhus and @ehmicky for patience and guiding me through the review :)

ehmicky · 2019-03-10T18:28:27Z

@tomsotte thanks for this nice addition!

sindresorhus mentioned this pull request Jan 22, 2019

Return a .all property in the result #172

Closed

ehmicky requested changes Mar 6, 2019

View reviewed changes

tomsotte added 3 commits March 7, 2019 09:53

Added all stream documentation and why it's not possible in execa.s…

97cde71

…ync()

More predictable noop-132 fixture for testing all stream output

23799a0

The delay 6000ms is approximate, it may not work on every machine.

tomsotte added 2 commits March 8, 2019 11:13

Merge with upstream/master; fix rename m -> execa in test.js

40aa251

Merge branch 'master' into iss1

3832328

tomsotte added 2 commits March 8, 2019 11:21

Fixed trailing space

e35dcdf

Using test.serial for testing result.all with lower timeout for noop-…

7f430d9

…132 (1000ms) Testing with test.serial and a delay of only 1000ms seems to pass multiple test runs consistently. Let's see if it passes on CI testing as well.

ehmicky reviewed Mar 9, 2019

View reviewed changes

test.js Outdated Show resolved Hide resolved

tomsotte added 2 commits March 9, 2019 15:26

Added execa.all(), similar to execa.stdout() for result.all; added test

b500215

Fixed link to Node.js docs about process I/O stream

a419b4a

tomsotte added 2 commits March 9, 2019 17:34

all available on errors, if available in result (ex. no execa.sync())

bc1450a

Added all interleaved stream to readme in Why, example and API

29f3f12

tomsotte changed the title ~~Added result.all intermixed stdout/stderr (concat for sync). Closes #1~~ Added result.all intermixed stdout/stderr Mar 9, 2019

sindresorhus requested changes Mar 10, 2019

View reviewed changes

sindresorhus changed the title ~~Added result.all intermixed stdout/stderr~~ Add .all result property with interleaved stdout/stderr Mar 10, 2019

sindresorhus changed the title ~~Add .all result property with interleaved stdout/stderr~~ Add .all result property with interleaved stdout and stderr Mar 10, 2019

tomsotte added 3 commits March 10, 2019 15:40

Removed unnecessary comments on noop-132 and test.js

43a43bd

Fixed and improved readme about all interleaved stream

dc68492

Removed execa.all()

360bdb4

sindresorhus added 4 commits March 10, 2019 23:46

Update readme.md

06fbfff

Update readme.md

e128464

Merge branch 'master' into iss1

e842c72

Update readme.md

b815a79

sindresorhus approved these changes Mar 10, 2019

View reviewed changes

sindresorhus changed the title ~~Add .all result property with interleaved stdout and stderr~~ Add .all property with interleaved stdout and stderr Mar 10, 2019

sindresorhus merged commit 7f8d911 into sindresorhus:master Mar 10, 2019

ehmicky mentioned this pull request Mar 11, 2019

Add execa.all() #192

Closed

ehmicky mentioned this pull request May 15, 2019

v2 release post and changelog #253

Closed

ehmicky mentioned this pull request Nov 21, 2019

execa.sync doesn't (can't?) provide all #391

Closed

dependabot bot mentioned this pull request Mar 14, 2021

Bump @types/execa from 0.9.0 to 2.0.0 mike-north/devcert#37

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `.all` property with interleaved stdout and stderr #171

Add `.all` property with interleaved stdout and stderr #171

tomsotte commented Jan 20, 2019 •

edited

Loading

sindresorhus commented Mar 6, 2019

ehmicky Mar 6, 2019

tomsotte Mar 7, 2019

ehmicky Mar 7, 2019

tomsotte Mar 8, 2019

ehmicky Mar 8, 2019

ehmicky commented Mar 6, 2019

sindresorhus commented Mar 7, 2019

tomsotte commented Mar 8, 2019

tomsotte commented Mar 9, 2019

ehmicky commented Mar 9, 2019

tomsotte commented Mar 9, 2019

ehmicky commented Mar 9, 2019

sindresorhus commented Mar 10, 2019

sindresorhus commented Mar 10, 2019

tomsotte commented Mar 10, 2019

ehmicky commented Mar 10, 2019

Add .all property with interleaved stdout and stderr #171

Add .all property with interleaved stdout and stderr #171

Conversation

tomsotte commented Jan 20, 2019 • edited Loading

sindresorhus commented Mar 6, 2019

ehmicky Mar 6, 2019

Choose a reason for hiding this comment

tomsotte Mar 7, 2019

Choose a reason for hiding this comment

ehmicky Mar 7, 2019

Choose a reason for hiding this comment

tomsotte Mar 8, 2019

Choose a reason for hiding this comment

ehmicky Mar 8, 2019

Choose a reason for hiding this comment

ehmicky commented Mar 6, 2019

sindresorhus commented Mar 7, 2019

tomsotte commented Mar 8, 2019

tomsotte commented Mar 9, 2019

ehmicky commented Mar 9, 2019

tomsotte commented Mar 9, 2019

ehmicky commented Mar 9, 2019

sindresorhus commented Mar 10, 2019

sindresorhus commented Mar 10, 2019

tomsotte commented Mar 10, 2019

ehmicky commented Mar 10, 2019

Add `.all` property with interleaved stdout and stderr #171

Add `.all` property with interleaved stdout and stderr #171

tomsotte commented Jan 20, 2019 •

edited

Loading