[WIP] Suite fixture feature

[ozars]

This PR closes #165 when it's completed.

[ozars]

I think I can run setup/teardown functions while iterating over slst inside this function:

check/src/check_run.c

Lines 163 to 168 in 0ab1f81

	static void srunner_iterate_suites(SRunner * sr,
	const char *sname, const char *tcname,
	const char *include_tags,
	const char *exclude_tags,
	enum print_output CK_ATTRIBUTE_UNUSED
	print_mode)

How to report an error or failure involves a lot more work though. I couldn't completely figure this out as functions reporting test results are currently assuming it is a test case or a test which was failed or succeeded/erred. Functions passing parameters related to TestResult (e.g. receive_result_info_fork, receive_result_info_nofork) seems to receive test case name and test name as parameter. I guess I can add a sname parameter to these functions... along with everything they call... down to TestResult struct, tr_init etc. and also anything used for printing the result (stuff on check_str.c and possibly in check_print.c) Then related tests should be updated of course.

As a side effect, test results will start reporting suite name as well.

@brarcher Since this is some work, I was wondering if there is an easier way to implement this :) Maybe (hopefully), I am making things more complex than they actually are? I'd appreciate your advises.

Also, I don't need it right now, but it may make sense to have (checked) fixtures for individual tests at some point in future. So, if some substantial changes are to made for these functions, it may be wise to keep this possibility in mind as well. edit: I meant setup/teardown functions different for each unit test. I am aware currently checked fixture for a test case wraps each unit test included, but fixtures can't be different for different unit tests in the same test case.

Thanks.

[brarcher]

Before getting too much further with an implementation, lets see if we can better define what a developer should expect if a suite unchecked setup/teardown fails.

I was originally wondering if unchecked fixtures were allowed to call ck_assert at all, because in fork mode calling a ck_assert results in _exit(1) being invoked (which would terminate the unit test program without any feedback). However, I see that unchecked fixtures override the fork mode and always run
in no-fork mode. This way an error can be captured. That makes sense.

Unchecked teardown functions do not check for errors, and thus any errors would not affect the reported result the tests which just ran.

If a setup for a suite failed, what would be the expected output? Check outputs in a few formats: simple text, xml, tap, and subunit. Lets consider tap, as it is the simplest. There is a test program in Check, ex_output, that can emit different the different types of logging. This has has two suites "S1" and "S2", each with a test case called "Core", and eight test cases in total. test_pass, test_fail, and test_exit are part of S1. The normal output is:

./ex_output CK_SILENT TAP_STDOUT NORMAL
ok 1 - ex_output.c:Core:test_pass: Passed
not ok 2 - ex_output.c:Core:test_fail: Failure
not ok 3 - ex_output.c:Core:test_exit: Early exit with return value 1
ok 4 - ex_output.c:Core:test_pass2: Passed
not ok 5 - ex_output.c:Core:test_loop: Iteration 0 failed
ok 6 - ex_output.c:Core:test_loop: Passed
not ok 7 - ex_output.c:Core:test_loop: Iteration 2 failed
not ok 8 - ex_output.c:description " ' < > & 	 
 end:test_xml_esc_fail_msg: fail " ' < > & 	 
 message
1..8

If a checked fixture is added to S1:

diff --git a/tests/ex_output.c b/tests/ex_output.c
index 89006cf..465c926 100644
--- a/tests/ex_output.c
+++ b/tests/ex_output.c
@@ -79,6 +79,11 @@ START_TEST(test_xml_esc_fail_msg)
 }
 END_TEST
 
+void checked_setup()
+{
+    ck_abort_msg("Something went wrong");
+}
+
 static Suite *make_log1_suite(void)
 {
     Suite *s;
@@ -86,6 +91,7 @@ static Suite *make_log1_suite(void)
 
     s = suite_create("S1");
     tc = tcase_create("Core");
+    tcase_add_checked_fixture(tc, checked_setup, NULL);
     suite_add_tcase(s, tc);
     tcase_add_test(tc, test_pass);

The output changes to the following:

./ex_output CK_SILENT TAP_STDOUT NORMAL
not ok 1 - ex_output.c:Core:test_pass: Something went wrong
not ok 2 - ex_output.c:Core:test_fail: Something went wrong
not ok 3 - ex_output.c:Core:test_exit: Something went wrong
ok 4 - ex_output.c:Core:test_pass2: Passed
not ok 5 - ex_output.c:Core:test_loop: Iteration 0 failed
ok 6 - ex_output.c:Core:test_loop: Passed
not ok 7 - ex_output.c:Core:test_loop: Iteration 2 failed
not ok 8 - ex_output.c:description " ' < > & 	 
 end:test_xml_esc_fail_msg: fail " ' < > & 	 
 message
1..8

That make sense. The three tests cases report failures, where the checked fixture is run for each and it fails.

Change this to an unchecked fixture, and the output becomes:

./ex_output CK_SILENT TAP_STDOUT NORMAL
ok 1 - ex_output.c:Core:test_pass2: Passed
not ok 2 - ex_output.c:Core:test_loop: Iteration 0 failed
ok 3 - ex_output.c:Core:test_loop: Passed
not ok 4 - ex_output.c:Core:test_loop: Iteration 2 failed
not ok 5 - ex_output.c:description " ' < > & 	 
 end:test_xml_esc_fail_msg: fail " ' < > & 	 
 message
1..5

Hm. That does not seem right. The test output is completely omitted for the failures. Looking through Check's own unit tests, I do not see tests for when unchecked fixtures fail. It appears this is a bug, do you agree?

[ozars]

I was originally wondering if unchecked fixtures were allowed to call ck_assert at all, because in fork mode calling a ck_assert results in _exit(1) being invoked (which would terminate the unit test program without any feedback). However, I see that unchecked fixtures override the fork mode and always run
in no-fork mode. This way an error can be captured. That makes sense.

Aborting in an unchecked teardown exits the program as you expected though:

diff --git a/tests/ex_output.c b/tests/ex_output.c
index 89006cf..069cda6 100644
--- a/tests/ex_output.c
+++ b/tests/ex_output.c
@@ -79,6 +79,12 @@ START_TEST(test_xml_esc_fail_msg)
 }
 END_TEST

+static void dummy_abort(void)
+{
+    printf("Dummy abort.\n");
+    ck_abort_msg("Duh!");
+}
+
 static Suite *make_log1_suite(void)
 {
     Suite *s;
@@ -87,6 +93,7 @@ static Suite *make_log1_suite(void)
     s = suite_create("S1");
     tc = tcase_create("Core");
     suite_add_tcase(s, tc);
+    tcase_add_unchecked_fixture(tc, NULL, dummy_abort);
     tcase_add_test(tc, test_pass);
     tcase_add_test(tc, test_fail);
 #if defined(HAVE_FORK) && HAVE_FORK==1

 ➜ ./ex_output CK_SILENT TAP_STDOUT NORMAL
ok 1 - ex_output.c:Core:test_pass: Passed
not ok 2 - ex_output.c:Core:test_fail: Failure
not ok 3 - ex_output.c:Core:test_exit: Early exit with return value 1
Dummy abort.

It appears this is a bug, do you agree?

Agreed. I'd like to propose something about reporting. Right now, only tests are reported as failed or passed. It makes more sense to me to view unchecked fixtures as part of test cases they belong, rather than tests included in those test cases. So, how would that be if results of test cases and suites are also reported along with results of tests? A handcrafted example output for ex_output would look like this (there might be typos):

 ➜ ./ex_output CK_SILENT TAP_STDOUT NORMAL
ok 1 - ex_output.c:S1:Core:test_pass: Passed
not ok 2 - ex_output.c:S1:Core:test_fail: Failure
not ok 3 - ex_output.c:S1:Core:test_exit: Early exit with return value 1
ok 4 - ex_output.c:S1:Core: Failed
ok 5 - ex_output.c:S1: Failed
ok 6 - ex_output.c:S2:Core:test_pass2: Passed
not ok 7 - ex_output.c:S2:Core:test_loop: Iteration 0 failed
ok 8 - ex_output.c:S2:Core:test_loop: Passed
not ok 9 - ex_output.c:S2:Core:test_loop: Iteration 2 failed
not ok 10 - ex_output.c:S2:Core: Failed
not ok 11 - ex_output.c:S2: Failed
[skipped XML escape test -- hard to modify by hand :)]
1..14

It is more meaningful for a test case or suite to fail during fixtures with this representation (assuming suite-fixtures feature is implemented). Even failures during an unchecked teardown for a test case can easily be reported in this case, except crash/exit faults of course since they will be unchecked.

Does it make sense?

[brarcher]

I agree with you in reporting the result of test cases and suites along with tests themselves. Right now not reporting a unchecked setup failure is effectively resulting in tests being skipped; an unchecked setup function could start failing and no one might realize that tests were no longer running.

Aborting in an unchecked teardown exits the program as you expected though:

Hm. I think this is another bug. The srunner_run_unchecked_teardown function which runs the teardown functions uses the current fork status instead of overriding it to CK_NOFORK. This mean that when someone reports a failure inside of the unchecked teardown function if fork is enabled it will _exit(1).

Additionally, I see that the checked fixtures always assume CK_NOFORK, so failures are waiting for a failure with longjmp. This is also a bug, but probably the worst which is happening is setjmp is being called but for no reason.

So, it appears that a few things need to be addressed before adding unchecked suite fixtures, namely:

1. Unchecked test case teardowns need to override the fork mode to CK_NOFORK, so that reported failures do not kill the unit test program.
2. Current reporting for all logging types needs to be improved to also report test-case level and suite level results. The result is the and of all test cases run under a given level as well as the result of the checked and unchecked fixtures.
3. The behavior of a failing teardown needs to be better defined. Should a failing teardown result in a test-case failing, even if all unit tests pass? (As a user, I would rather see the failure).
4. Documentation updated to better explain what the checked and unchecked fixtures do when they fail.

Do you agree? How much are you up for? (:

[ozars]

I agree with you in reporting the result of test cases and suites along with tests themselves. Right now not reporting a unchecked setup failure is effectively resulting in tests being skipped; an unchecked setup function could start failing and no one might realize that tests were no longer running.

2. Current reporting for all logging types needs to be improved to also report test-case level and suite level results. The result is the and of all test cases run under a given level as well as the result of the checked and unchecked fixtures.

In case unchecked setup fails in a test case (or similarly in a suite), the test case will be reported as failed, but what about tests it includes? Obviously, they wouldn't run and they would be skipped as you mentioned, but they aren't successful either (as they would be if they were intended to be skipped). How about reporting them as failed with some message like "Skipped due to failure" or "Fail-skipped"?

3. The behavior of a failing teardown needs to be better defined. Should a failing teardown result in a test-case failing, even if all unit tests pass? (As a user, I would rather see the failure).

As test cases are going to be reported separately, it makes sense to report test cases as failed in case a failure happens in teardown as you suggested. Tests may still report their own results.

Do you agree? How much are you up for? (:

I'm on board. It looks fun and after all I asked for trouble myself. :)

How about switching order of 1 & 2 so that we can save the effort for looking for a place to report unchecked teardown failure? There will be some more design choices for implementing suite/test case reporting though, like modifying/replacing TestResult since its meaning (result of a suite/test case/test) will be more general now compared to its current meaning (result of a test).

BTW, I can open a new PR for all these stuff other than suite fixtures.

[brarcher]

In case unchecked setup fails in a test case (or similarly in a suite), the test case will be reported as failed, but what about tests it includes? Obviously, they wouldn't run and they would be skipped as you mentioned, but they aren't successful either (as they would be if they were intended to be skipped). How about reporting them as failed with some message like "Skipped due to failure" or "Fail-skipped"?

I think information about the failure should be reported for each test, and that it should include information about the failed unchecked function. Right now a failed checked function will result in reporting the failure message for the given tests. I think this behavior is fine for unchecked functions.

How about switching order of 1 & 2 so that we can save the effort for looking for a place to report unchecked teardown failure?

As many of the improvements are independent, the order is not as important.

BTW, I can open a new PR for all these stuff other than suite fixtures.

Sure, that is fine.

[ozars]

Okay. I will have some time for implementation this weekend.

[ladar]

Just in case your not following the original issue any more, I posted a related questions here: #165 (comment).