-
Notifications
You must be signed in to change notification settings - Fork 23
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 vipgoci_sysexit() unit test #200
Conversation
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.
This is good progress towards having more code coverage.
Ideally, modifying the original function should not be needed to make the test pass/work. The tests should always test against the actual application behavior. However, with the exit
construct usage, I understand it's not possible for the moment.
Improvement suggestions for the future:
- Replace the
vipgoci_sysexit
with avipgoci_build_get_sys_exit_status
, add $exit_status/irc logic there; - Test the
vipgoci_build_get_sys_exit_status
function; - Leave the exit call untested since it would have no logic implemented;
- Replace the
exit
construct call with throwables:
- The implementation: Create a specific Exception for exit errors; To start, we can try/catch in the main function; (and then, later implement OOP, DI, etc.), call the exit with the code;
- The goal:
try { $codeAnalyser->run(); } catch($e) { exit($e->getCode()); }
- Upside: Helps us to move towards making vip-go-ci code more testable and readable;
- Downside: this would affect the entire application;
Having that said, I believe that having this test that does the job helps us move towards more code coverage.
So, approved. :)
* with exit status. | ||
*/ | ||
if ( | ||
( function_exists( 'vipgoci_unittests_check_indication_for_test_id' )) && |
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.
Ideally, it should not be needed to modify the original function to make the test pass/work. The tests should always test against the actual application behavior. However, in this context, with the exit
usage, I understand it's not possible for the moment.
Thanks for the review and suggestions! I wanted to provide some commentary on the suggestions. Regarding 2) The program is procedural and switching to OOP would require a rewrite. I'm uncertain of the benefits in relation to the effort, especially as the aim here is more unit testing coverage. Also, the program is not exiting based purely on the results of one type scanning, but based on the whole picture of the results. I'm skeptical of the approach here. Regarding 1) I'm uncertain of removing Generally, I agree that the solution is not ideal, and I would like the solution to be more ideal. However, given our constraints regarding time and given the life-cycle of the program, I think we will have to accept that some solutions will never be fully ideal. |
This patch will add a unit-test to test
vipgoci_sysexit()
.TODO:
vipgoci_sysexit()