fsquillace / junest

The lightweight Arch Linux based distro that runs, without root privileges, on top of any other Linux distro.
GNU General Public License v3.0
2.08k stars 111 forks source link

Limit blast radius of eval to JUNEST_ARGS #289

Closed neiser closed 2 years ago

neiser commented 2 years ago

A probably better approach to #262 and #287 is to only eval the JUNEST_ARGS variable. This commit achieves exactly this.

fsquillace commented 2 years ago

Cool, sounds good! Also, one thing important is to make sure we have a better tests to avoid have this issue repeating. Here is where we call the wrapper using a simple command execution: https://github.com/fsquillace/junest/blob/master/lib/checks/check_all.sh#L25

Maybe it would be good to have a more complex test with a custom command passing nested quotes arguments and doing some assertions into it, wdyt?

neiser commented 2 years ago

Yes, I thought about adding tests but it seemed too tedious to run them locally (and waiting for the Travis pipeline is just too bad dev ux for me 😉). Did I miss some docs which explain to me how to run tests quickly on my machine? Maybe I'm willing to add some more elaborate test for the wrapper script.

fsquillace commented 2 years ago

The unit tests can be run in this way: https://github.com/fsquillace/junest/blob/master/CONTRIBUTING.md#unit-tests

There is a test-wrapper file which is the only one that should be changed. You can execute it directly too (without running the whole test suite).

fsquillace commented 2 years ago

Try to add a test. If you cannot I will attempt to add it at a second stage. Thanks!

fsquillace commented 2 years ago

Merging the change with unit tests: https://github.com/fsquillace/junest/commit/73b8bec8db34d1cfb51bf97934ac31a3909f0eaf