LLNL / ATS

ATS - Automated Testing System - is an open-source, Python-based tool for automating the running of tests of an application across a broad range of high performance computers.
BSD 3-Clause "New" or "Revised" License
6 stars 5 forks source link

Exit code bugfix #129

Closed mdavis36 closed 1 year ago

mdavis36 commented 1 year ago

I believe the returned values from the management.py main method needs to be swapped. I have noticed ATS report an exit code of 1 with successful runs.

dawson6 commented 1 year ago

The wording is confusing I agree. But looking at this for consitency sake. That is there are projects which return the results from the 'core' routine as well, and that is also returning True for success and False for errors. So this change would be inconsistent with that coding.

As True is 1 and False is 0 in Python. If we want the main to return a (0) on sucess and (non-zero, or 1) on failure, then we should be explicit there perhaps. IE change the return to be a (0) or (1) rather than True or False.

Would something like this be better for 'main'

    if (core_result and postprocess_result and report_result):
        return 0
    else:
        return 1
dawson6 commented 1 year ago

We need to see how projects are using this. At one point @white238 pointed out return value inconsistencies as well, so we need to be careful not to flip a return value that projects may currently depend on.

That is, if 'main' is called from a python layer, somehow it may depend on getting 'True' for the return value. If from the command line then (0) makes sense. The confusion is that 0 is False.

dawson6 commented 1 year ago

Yes, thinking about this more, we have use cases where 'ats' is wrapped with other python code, which may depend on current return value of True. So while I believe this would work for my projects use, and others, there may be a project depending on True being returned.

We can give it a try but need to advertise the change well before releasing.

And if possible have it explicitly return(0) rather than False if we can.

white238 commented 1 year ago

Programs should return 0 on a successful run and any other number should be treated as an error code.

Also since this never returned a value before, it is should be safe to return whatever you like.

dawson6 commented 1 year ago

Thanks @white238.

OK, with that input, I can push a change to this MR to have it (0) (not False) on success and (1) (not True) on failure.

dawson6 commented 1 year ago

closing MR and resubmitting.