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

Limit blast radius of eval to JUNEST_ARGS #289

Closed
wants to merge 1 commit into from

Conversation

neiser
Copy link
Contributor

@neiser neiser commented Feb 13, 2022

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

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

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
Copy link
Contributor Author

neiser commented Feb 14, 2022

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
Copy link
Owner

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
Copy link
Owner

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

fsquillace pushed a commit that referenced this pull request Apr 24, 2022
fsquillace pushed a commit that referenced this pull request Apr 24, 2022
@fsquillace
Copy link
Owner

Merging the change with unit tests: 73b8bec

@fsquillace fsquillace closed this Apr 24, 2022
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