-
Notifications
You must be signed in to change notification settings - Fork 13
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
Use zsh as shell for macOS integration tests. #2883
Conversation
mitchell-as
commented
Nov 13, 2023
•
edited by github-actions
bot
Loading
edited by github-actions
bot
DX-2142 Our macOS integration tests use zsh, not bash |
5f0c75b
to
177dada
Compare
9233f42
to
7319fe3
Compare
7319fe3
to
28f0ab8
Compare
Previous "Test: all" was successful except for one installer test that has since been fixed. I just didn't want to wait another 40 minutes to confirm it passed. |
@@ -212,7 +208,7 @@ func (suite *InstallerIntegrationTestSuite) TestInstallerOverwriteServiceApp() { | |||
) | |||
cp.Expect("Done") | |||
cp.SendLine("exit") | |||
cp.ExpectExitCode(0) | |||
cp.ExpectExit() // the return code can vary depending on shell (e.g. zsh vs. bash); just assert the installer shell exited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this type of check be necessary in other integration tests now?
It may be worth considering a new function that wraps ExpectExitCode()
and ExpectExit()
and uses either one depending on the current shell, but I will leave that up to your discretion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could rear its head in other installer tests, but in all of our other subshell tests that use "exit" followed by "ExpectExitCode(0)", the tests pass. Nathan found a similar issue with shells on Windows: 3d87ecb. It's something we'll have to live with I guess.