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

Add vipgoci_sysexit() unit test #200

Merged
merged 10 commits into from
Sep 9, 2021
Merged

Add vipgoci_sysexit() unit test #200

merged 10 commits into from
Sep 9, 2021

Conversation

gudmdharalds
Copy link
Contributor

@gudmdharalds gudmdharalds commented Aug 18, 2021

This patch will add a unit-test to test vipgoci_sysexit().

TODO:

  • Add/update unit-test for vipgoci_sysexit()
  • Changelog entry
  • Check automated unit-tests
  • Add or update PHPDoc comments for new or updated functions.
  • Changelog entry [Changelog for version 1.0.9 #206]

@gudmdharalds gudmdharalds added this to the 1.0.9 milestone Aug 25, 2021
Copy link
Member

@ariskataoka ariskataoka left a 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 a vipgoci_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;
  1. 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' )) &&
Copy link
Member

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.

@gudmdharalds gudmdharalds merged commit aa91b70 into main Sep 9, 2021
@gudmdharalds gudmdharalds deleted the add-sysexit-unit-test branch September 9, 2021 11:29
@gudmdharalds
Copy link
Contributor Author

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 exit() from vipgoci_sysexit() and move it to other places in the code, because this could easily lead to problems in the 40+ places in the code where the vipgoci_sysexit() function is used. I don't think adding unit testing coverage is worth it for such a simple function when mistakes could be introduced elsewhere.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants