fix: nonzero return values should produce a fail

[umarcor]

Related to #465.

If a GHDL simulation is wrapped in a C application, when the simulation is successful but the application exists with a nonzero code, VUnit still reports the test as passed. This is because it ignores the return code and checks the output file instead.

This PR sets the status of a test to fail if the return code is nonzero.

[kraigher]

VUnit ignores the return code to be able to end simulation with an assert of severity failure for non VHDL-2008 where std.end.stop exists. Thus failing all test when the exit code is non-zero will break VUnit-support for VHDL 93 and 2002.

[umarcor]

What is your suggestion?

• Enable this fix only when VHDL 2008 is used.
• Find a different mechanism to tell VUnit when some wrapper of the sim does want to report a failure.

[kraigher]

An acceptable way forward would be to check the simulator exit code in VHDL 2008. It would prevent people wrapping the simulator binary from using a standard less than 2008 but I guess that would be no problem.

[umarcor]

An acceptable way forward would be to check the simulator exit code in VHDL 2008.

I now added a check for the version.

It would prevent people wrapping the simulator binary from using a standard less than 2008 but I guess that would be no problem.

If someone tries to wrap the binary with a standard less than 2008, the behaviour will be the same as if no fix existed. So, as far as I understand it, they can still do it, but they will have to handle false positives.

[umarcor]

@kraigher this is failing because lib.tb_same_sim_some_fail.Test 1 got failed expected passed. Should I change the expected value to failed?

[kraigher]

The situation is that VUnit supports running all test cases within a test bench in the same simulation using the run_all_in_same_sim comment attribute. In these cases you might have three tests "Test 1", "Test 2" and "Test 3" that are run in sequence. If "Test 1" pass and "Test 2" fails the "Test 3" will be considered skipped as the simulation stops at the failure of "Test 2". The change will make "Test 1" fail also since the exit code will fail all tests that were run in the same simulation.

I think the acceptance criteria is correct and should not be changed. One way to solve it would be to only fail all tests if all test pass in the presence of non-zero exit code. Thus if any test fails we let the test results be the way they where even if the exit code was non-zero. This would behave the same way as the current solution for the default case where each test is run in its own simulation.

We have a lot of acceptance test in VUnit to ensure things we wanted to work a specific way does not unintentionally break in the future. In this case your PR does not include any comment, unit test or acceptance test so there is a likelyhood that the behavior you want w.r.t the exit code will break in the future. Maybe a unit test with a comment is sufficient in this case.

[umarcor]

I think the acceptance criteria is correct and should not be changed. One way to solve it would be to only fail all tests if all test pass in the presence of non-zero exit code. Thus if any test fails we let the test results be the way they where even if the exit code was non-zero. This would behave the same way as the current solution for the default case where each test is run in its own simulation.

I moved the fix after the check now.

I am not sure if the check should be kept as status != PASSED or changed to status == FAILED. ATM, if some pass and others are skipped, the check of nonzero exit code is not evaluated.

We have a lot of acceptance test in VUnit to ensure things we wanted to work a specific way does not unintentionally break in the future. In this case your PR does not include any comment, unit test or acceptance test so there is a likelyhood that the behavior you want w.r.t the exit code will break in the future. Maybe a unit test with a comment is sufficient in this case.

I'm trying to add some unit test, but I am finding it difficult to implement without actually compiling a C file where GHDL is embedded. Any ideas?

EDIT

If found that p_simulation_exit_is_disabled is checked in test_runner_cleanup. So, if I can set it to enabled, I can skip VUnit terminating the simulation.

[kraigher]

The rule should be, if non zero exit code and not any status=FAILED and code we flip all PASSED to FAILED

Regarding unit test it should be a python test in test/unit/test_test_suites.py at the function level without involving any external tool. This is unlike an acceptable test where VUnit is tested end to end and the simulator is involved.

[umarcor]

The rule should be, if non zero exit code and not any status=FAILED and code we flip all PASSED to FAILED

Agree. Moreover, now all the tests must be PASSED to execute call_post_check. Would it be acceptable to allow PASSED and/or SKIPPED to be passed to call_post_check instead?

Regarding unit test it should be a python test in test/unit/test_test_suites.py at the function level without involving any external tool. This is unlike an acceptable test where VUnit is tested end to end and the simulator is involved.

Isn't it possible to do something similar to p_disable_simulation_exit(runner)? To do so, I need some 'conversion' between runner_sync_t (the one used in the testbench) and runner_t (the one accepted by the function). Furthermore, the function or a wrapper would need to be made public.

I added tb_same_sim_all_pass_nonzero.vhd and a test to test_artificial.py (_test_run_selected_tests_in_same_sim_test_bench). I tried two possible implementations to avoid test_runner_cleanup terminating the simulation. The first one is calling p_disable_simulation_exit explicitly:

      end if;
    end loop;
    vunit_lib.runner_pkg.p_disable_simulation_exit(runner_state);
    test_runner_cleanup(runner);
    assert false severity error;
    wait;
  end process;
end architecture;

The other one, which is the proposed one, is to add option do_not_exit to test_runner_cleanup:

      end if;
    end loop;
    test_runner_cleanup(runner, do_not_exit => true);
    assert false severity error;
    wait;
  end process;
end architecture;

I thought that this feature might be implemented already. Maybe through the Python API, instead of the VHDL package. However, I could not find it.

Alternatively, I think I could use a post_run, which I believe is what you mean with at the function level without involving any external tool.

Additionally, I moved the added code to a function named _check_results. This makes it easier to add tests to test_test_suites, which I also did.

[kraigher]

The major things I want to change is that I think a unit test is enough and would like to avoid adding another acceptance test.

Also I think there are missing scenarios in the unit test. See my comments.

[umarcor]

I applied changes according to the comments above. Now, the following scenarios are tested:

• {"test1": PASSED}
• {"test1": PASSED, "test2": SKIPPED}
• {"test1": FAILED, "test2": SKIPPED}
• {"test1": PASSED, "test2": FAILED}
• {"test1": PASSED, "test2": FAILED, "test3": SKIPPED}

For each of them, four different tests are executed:

• sim_ok=True && has_valid_exit_code=False, regular zero exit.
• sim_ok=False && has_valid_exit_code=False, regular nonzero exit.
• sim_ok=True && has_valid_exit_code=True, new zero exit.
• sim_ok=False && has_valid_exit_code=True, new nonzero exit. This is the only case where results might be overwritten.

I also added a commit where the style of the other tests is modified, just to reduce verbosity. Functionality is untouched.

[umarcor]

VUnit ignores the return code to be able to end simulation with an assert of severity failure for non VHDL-2008 where std.end.stop exists.

I am hitting this issue while testing #476 with vhdl_standard="93". A normal execution with VUnit is partially correct:

• The C code before the simulation is executed.
• The simulation is executed.
• When the simulation exists, the wrapper exists, so no C post-checks are executed.
• The python script does not exit.

Running test: lib.tb_external_buffer.test
Running 1 tests

Starting lib.tb_external_buffer.test
Output file: vunit_out/test_output/lib.tb_external_buffer.test_2f235d631488b2560e734c8488d8d48e8d0cd49e/output.txt
0: 11
1: 22
2: 33
3: 44
4: 55
5: 0
6: 0
7: 0
8: 0
9: 0
10: 0
11: 0
12: 0
13: 0
14: 0
               0 fs - default              -    INFO - Init test
               0 fs - default              -    INFO - End test
/src/vunit/vhdl/core/src/stop_body_93-2002.vhd:10:5:@0ms:(report failure): Stopping simulation with status 0
/src/examples/vhdl/external_buffer/vunit_out/test_output/lib.tb_external_buffer.test_2f235d631488b2560e734c8488d8d48e8d0cd49e/ghdl/tb_
external_buffer-tb:error: report failed
/src/examples/vhdl/external_buffer/vunit_out/test_output/lib.tb_external_buffer.test_2f235d631488b2560e734c8488d8d48e8d0cd49e/ghdl/tb_
external_buffer-tb:error: simulation failed
pass (P=1 S=0 F=0 T=1) lib.tb_external_buffer.test (0.4 seconds)

==== Summary =======================================
pass lib.tb_external_buffer.test (0.4 seconds)
====================================================
pass 1 of 1
====================================================
Total time was 0.4 seconds
Elapsed time was 0.4 seconds
====================================================
All passed!
EXCEPT
HELLO

But, when the binary is loaded dynamically from Python through ctypes, the execution is aborted:

• The C code before the simulation is executed.
• The simulation is executed.
• When the simulation exists, the wrapper receives a SIGABRT, which can be captured. So C post-checks can be potentially executed.
• However, Aborted is printed (I don't know who prints it) and the python script is terminated.

0: 11
1: 22
2: 33
3: 44
4: 55
5: 127
6: 0
7: 0
8: 176
9: 255
10: 41
11: 1
12: 0
13: 0
14: 0
               0 fs - default              -    INFO - Init test
               0 fs - default              -    INFO - End test
/src/vunit/vhdl/core/src/stop_body_93-2002.vhd:10:5:@0ms:(report failure): Stopping simulation with status 0
/src/examples/vhdl/external_buffer/vunit_out/test_output/lib.tb_external_buffer.test_2f235d631488b2560e734c8488d8d48e8d0cd49e/ghdl/tb_
external_buffer-tb:error: report failed

SIGABRT caught!
Aborted

On the one hand, I wonder whether it is possible to capture the exit in the C code for a normal VUnit simulation. It is surprising to me that SIGABRT is not used. Nonetheless, if I could capture when ghdl_main exits, it would be possible to run post-checks in C even with VHDL <2008. This is possible with atexit(my_exit_handler);

On the other hand, it should be possible to catch the abortion in the shared library and allow python to continue. When data is allocated from python, it should not be relevant how the simulation exited, because data will be written already. Alternatively, I should test if the abortion is produced because an executable binary is loaded dynamically; i.e., compiling the binary as a shared library explicitly might fix this (ghdl/ghdl#800).

Note that none of this issues exist with vhdl_standard="2008".