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

DOP-4701: Preserve stdout in ShellCommandExecutor #1043

Merged
merged 1 commit into from
May 20, 2024
Merged

Conversation

i80and
Copy link
Collaborator

@i80and i80and commented May 9, 2024

Stories/Links:

DOP-4701

Notes

The actual fix is the one line change in commandExecutor.ts.

The annoying part was making the test work:

  • The existing test incorrectly mocked the child_process.exec() interface.

  • According to the node docs, the exception raised by util.promisify(child_process.exec()) will always have a stdout property. But just in case the exception get raised by something else in the try block, it'll fall back to returning '' as in the current behavior.

  • util.promisify(child_process.exec) has a custom promisifier (child_process.exec[util.promisify.custom]) that considerably changes the interface, but that jest loses when you call jest.mock('child_process'), and because of jest magic tomfoolery, I couldn't find any tidy way to restore that custom hook, which is needed to actually make the interface behave correctly.

    So I thought, you know what? Why mock? What value do we get by mocking child_process.exec besides introducing surface areas for bugs?

    So this PR just executes commandExecutor.execute(["echo 'stdout output'", "echo 'stderr output' >&2", 'exit 1']), which gives us everything we need to make sure ShellCommandExecutor actually works, using only shell builtins that we already require.

README updates

    • This PR introduces changes that should be reflected in the README, and I have made those updates.
    • This PR does not introduce changes that should be reflected in the README

Copy link

github-actions bot commented May 9, 2024

Your feature branch infrastructure has been deployed!

Your webhook URL is: https://mnd0km6q5c.execute-api.us-east-2.amazonaws.com/prod/webhook/githubEndpoint/trigger/build

For more information on how to use this endpoint, follow these instructions.

@i80and i80and force-pushed the preserve-stdout branch from 78683d6 to 3d41c88 Compare May 19, 2024 23:20
@i80and i80and changed the title Preserve stdout in ShellCommandExecutor DOP-4701: Preserve stdout in ShellCommandExecutor May 19, 2024
@i80and i80and force-pushed the preserve-stdout branch from 3d41c88 to ebe85b3 Compare May 19, 2024 23:30
@i80and i80and marked this pull request as ready for review May 19, 2024 23:30
@i80and i80and requested review from seungpark and mmeigs May 19, 2024 23:36
@i80and i80and force-pushed the preserve-stdout branch 2 times, most recently from 45fd955 to 98ccb67 Compare May 19, 2024 23:44
@i80and i80and force-pushed the preserve-stdout branch from 98ccb67 to 53b9387 Compare May 19, 2024 23:45
Copy link
Contributor

@mmeigs mmeigs left a comment

Choose a reason for hiding this comment

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

Thank you for the explanations!!

@i80and i80and merged commit 228c1fb into main May 20, 2024
9 checks passed
@i80and i80and deleted the preserve-stdout branch May 20, 2024 21:59
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