search

uyar / calico

GNU General Public License v3.0
8 stars 4 forks source link

Support "return" keyword to be used to specify exit status #12

Closed sahinakkaya closed 4 years ago

sahinakkaya commented 4 years ago

I recently realized that calico doesn't use return keyword to specify exit status of the program. The keyword was exit since the beginning and it has never changed The keyword was changed to exit in this commit but the testers keep using return for some reason(the reason is obvious actually). Including myself, the testers were unaware that return: 0 actually does nothing because it is ignored by calico and the exit status of test case defaults to 0 when exit keyword is not present. Moreover, they avoid writing programs which returns 1 because when they try to test it, they were seeing exit status: 1 (expected 0) even though they write return: 1 in the test specification.

I don't know if ignoring unknown keywords silently has some purpose so I didn't want to touch that part. Instead, I modified related parts of code to support return keyword so that it can be used as intended.

I run the tests with Python3.8 and only the ones that are related with system calico have been failed because I don't have calico installed.

============================================================================================ short test summary info =============================================================================================
FAILED tests/test_calico.py::test_installed_version_should_match_package_version - pkg_resources.DistributionNotFound: The 'calico' distribution was not found and is required by the application
FAILED tests/test_cli.py::test_cli_version_should_print_version_number_and_exit - pkg_resources.DistributionNotFound: The 'calico' distribution was not found and is required by the application
========================================================================================== 2 failed, 62 passed in 5.03s ==========================================================================================
ghost commented 4 years ago

That was a backward-incompatibility sloppiness on my part, I guess. The reason for testers still using "return" might be due the calico version installed on our university servers being old, although I haven't checked it. Nice work, thanks!

ghost commented 4 years ago

Re: ignoring keywords silently. IIRC, there was a sanity checker in the code that validated the yaml file. I can't verify this at the moment, I'll have to get back to it.

sahinakkaya commented 4 years ago

Moreover, they avoid writing programs which returns 1 ...

I finally found the related test file, it was this and was used to evaluate our assignments. However, it seems that the keyword was return by that time so I don't have any idea why they choose 0 to signify an error :)

In response to your comments, currently calico 1.1.2 is installed at ITU's servers and it uses exit keyword since version 1.0. I think the reason why testers use return is because they are copy pasting from older test files whenever they want to write new tests. Considering this, #9 might be still a good idea. I will send a PR when I have time.