-
Notifications
You must be signed in to change notification settings - Fork 3k
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 the ERL_SYS_SHELL env variable to point to system shell for os:cmd() #8972
base: maint
Are you sure you want to change the base?
Conversation
CT Test Results 2 files 69 suites 1h 4m 55s ⏱️ For more details on these failures, see this check. Results for commit 98e2413. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
Hello! Thanks for the PR! The variable should be a kernel configuration parameter that is set to the correct value when kernel is started. It also needs to be documented here and a testcase needs to be added to os_SUITE. |
f3e7b34
to
94dde80
Compare
Thanks for the update! I've pushed a commit that renames the variable to Would you mind updating the docs of os:cmd/2 to point to the docs for the configuration parameter so that users know where to find it? Feel free to squash my commit into yours if you agree with my changes. |
… for os:cmd/2 Initialize the parameter at startup Fix the test - restore the old shell after the test Add initialization to other tests that use os:cmd/2 Update os:cmd/2 description
f71fc7a
to
7af29a1
Compare
I'm a bit confused by the test results. As I see one subtest in shell_history_custom_error/1 test failed with the timeout, but a similar subtest passed just fine. Moreover I don't see how this part of the functionality can be affected by my changes, and locally the test passed just fine. Could you give me a hint on what might cause the test to fail? |
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.
I think there might be a race in that testcase that you've had the misfortune of hitting, so you can ignore that.
However, the os:internal_init_os_cmd/1 should not need to be called in all those test suites. The testcase needs to make sure that it resets the state (as you have done). Why did you add them?
Also could you please add a test that uses ?CT_PEER to start a new code where the config parameter is set at startup to see that that works. I don't think os:cmd is used anywhere when booting Erlang so we should be safe.
Co-authored-by: Lukas Backström (FKA Larsson) <[email protected]>
I added os:internal_init_os_cmd/1 because locally the tests that contain os:cmd/2 (at least some of them) crashed with badmatch at |
How odd, can you remove them and see if that happens in github ci as well? |
A fix for #8944