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 .all property with interleaved stdout and stderr #171

Merged
merged 19 commits into from Mar 10, 2019

Conversation

tomsotte
Copy link
Contributor

@tomsotte tomsotte commented Jan 20, 2019

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
Copy link
Owner

@ehmicky Would you be able to help review this?

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@@ -0,0 +1,14 @@
#!/usr/bin/env node
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think @sindresorhus?

fixtures/noop-132 Outdated Show resolved Hide resolved
@ehmicky
Copy link
Collaborator

ehmicky commented Mar 6, 2019

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.
The delay 6000ms is approximate, it may not work on every machine.
@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

@tomsotte
Copy link
Contributor Author

tomsotte commented Mar 8, 2019

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
Copy link
Contributor Author

tomsotte commented Mar 9, 2019

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

test.js Outdated Show resolved Hide resolved
@ehmicky
Copy link
Collaborator

ehmicky commented Mar 9, 2019

@tomsotte this seems like a good idea :)

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

@tomsotte
Copy link
Contributor Author

tomsotte commented Mar 9, 2019

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
Copy link
Collaborator

ehmicky commented Mar 9, 2019

Yes nice catch 👍

@tomsotte tomsotte changed the title Added result.all intermixed stdout/stderr (concat for sync). Closes #1 Added result.all intermixed stdout/stderr Mar 9, 2019
index.js Outdated Show resolved Hide resolved
fixtures/noop-132 Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Added result.all intermixed stdout/stderr Add .all result property with interleaved stdout/stderr Mar 10, 2019
@sindresorhus sindresorhus changed the title Add .all result property with interleaved stdout/stderr Add .all result property with interleaved stdout and stderr Mar 10, 2019
@sindresorhus 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 sindresorhus merged commit 7f8d911 into sindresorhus:master Mar 10, 2019
@sindresorhus
Copy link
Owner

Thanks for contributing, @tomsotte :)

@sindresorhus
Copy link
Owner

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

@tomsotte
Copy link
Contributor Author

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

@ehmicky
Copy link
Collaborator

ehmicky commented Mar 10, 2019

@tomsotte thanks for this nice addition!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return an .all property with both stdout and stderr intermixed
3 participants