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

stdstream support #168

Merged
merged 9 commits into from
Sep 18, 2024
Merged

stdstream support #168

merged 9 commits into from
Sep 18, 2024

Conversation

ezrizhu
Copy link
Collaborator

@ezrizhu ezrizhu commented Jul 13, 2024

symlinks /dev/std{in,out,err} to /proc/self/fd/{0,1,2} and cleanup after
tests included

@ezrizhu ezrizhu self-assigned this Jul 13, 2024
@ezrizhu ezrizhu requested a review from mgree July 16, 2024 06:16
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

Nice! Could we add one more test that actually uses these streams, rather than just testing that the devices exist?

Here's a simple test script that reads a number x from stdin and outputs 2x on stdout and 3x on stderr:

read x
echo $((x * 2))
echo $((x * 3)) >&2

We should be able to run echo 5 | try $SCRIPT and get 10 on stdout and 15 on stderr. Similarly, we should be able to run echo 5 >FILE; try $SCRIPT <FILE (NB redir is in our shell, not in try's arg) and get the same result.

test/stdstream.sh Outdated Show resolved Hide resolved
try Show resolved Hide resolved
@ezrizhu
Copy link
Collaborator Author

ezrizhu commented Jul 22, 2024

Nice! Could we add one more test that actually uses these streams, rather than just testing that the devices exist?

Here's a simple test script that reads a number x from stdin and outputs 2x on stdout and 3x on stderr:

read x
echo $((x * 2))
echo $((x * 3)) >&2

We should be able to run echo 5 | try $SCRIPT and get 10 on stdout and 15 on stderr. Similarly, we should be able to run echo 5 >FILE; try $SCRIPT <FILE (NB redir is in our shell, not in try's arg) and get the same result.

That example would also pass outside of this PR since it seems like sh does not need /dev/std{in,out,err} for those streams. @mgree

@mgree
Copy link
Contributor

mgree commented Jul 23, 2024

That example would also pass outside of this PR since it seems like sh does not need /dev/std{in,out,err} for those streams. @mgree

Oh! Good point lol. Then we should write a test that uses those devices appropriately.

Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

I think this can be simpler and more precise.

EOF

chmod +x "$cmdfile"

result=$("$TRY" "$cmdfile")
expected=$(sh "$cmdfile")
# test stdout
Copy link
Contributor

Choose a reason for hiding this comment

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

why not test for exact output from both at once by capturing output in mktemped files?

@ezrizhu ezrizhu requested a review from mgree September 6, 2024 04:40
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

One more test please... 🫣

test/stdstream.sh Show resolved Hide resolved
try Show resolved Hide resolved
Copy link
Contributor

@mgree mgree left a comment

Choose a reason for hiding this comment

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

No diff on try stderr due to mount noise, I assume? LGTM thanks!

@ezrizhu ezrizhu merged commit 71cb569 into main Sep 18, 2024
21 checks passed
@ezrizhu ezrizhu deleted the stdstream branch September 18, 2024 03:18
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.

2 participants