Adamdebek/exit test #208

adamdebek · 2023-05-23T11:19:53Z

Description

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (list targets here).

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

libc/exit.c

niewim19

General, repeating remarks:

if clause on fork exit has no effect and is used in many testcases
variable declaration in the middle of the code
TEST_ASSERTs in fork-ed processes may produce strange unity behavior and give unclear output in test runner. If you need to assert something in child process then do it via if clause and pass failure information as exit status of that process.
Please add description to PR

niewim19 · 2023-06-22T13:42:59Z

exit/test_exit.c

+	int val[3];
+	val[0] = 0x1 << 8;
+	val[1] = (0x1 << 16) + 1;
+	val[2] = (0x1 << 24) + 2;


The same name as global variable val. Please describe logic that stands behind these values calculations.

This is done because only the least significant 8 bits shall be available to a waiting parent process. 3 most significant bytes should be cut off so returning val[0] should resolve to returning 0, val[1] to 1 and val[2] to 2.

exit/test_exit.c

niewim19 · 2023-06-22T15:33:07Z

exit/test_exit.c

+		for (i = 0; i < atexit_max - 1; i++) {
+			TEST_ASSERT_EQUAL_INT(0, atexit(fun1));
+		}
+		TEST_ASSERT_EQUAL_INT(0, atexit(fun2));
+		TEST_ASSERT_EQUAL_INT(0, atexit(fun1));


Why is fun2 used? Cant we try to register atexitMax + 1offun1and assert onlyatexitMax` invocations?

Last registered function is invoked first, so I created fun2 to make distinguish between function calls and ensure they are called in proper order.

Is this order asserted somewhere?

exit/test_exit.c

niewim19 · 2023-07-14T07:19:45Z

exit/test_exit.c

+		pid_t old_ppid, new_ppid;
+		int status;


variable declaration deep in test body, not in the beginning. Also names do not follow coding convention.

I think this approach is more readable and was used in other files in the past:

phoenix-rtos-tests/socket/test_socket.c

Lines 623 to 670 in 4334a5c

int fd[2];

pid_t pid;

size_t sfdcnt, rfdcnt;

sfdcnt = rand() % (MAX_FD_CNT + 1);

if (socketpair(AF_UNIX, type, 0, fd) < 0)

FAIL("socketpair");

if ((pid = fork()) < 0)

FAIL("fork");

if (pid) {

int sfd[MAX_FD_CNT];

ssize_t n;

int status;

if (open_files(sfd, sfdcnt) < 0)

FAIL("open_files");

if (write_files(sfd, sfdcnt) < 0)

FAIL("write_files");

n = unix_msg_send(fd[0], data, 1, sfd, sfdcnt);

TEST_ASSERT(n == 1);

if (close_files(sfd, sfdcnt) < 0)

FAIL("close_files");

if (waitpid(pid, &status, 0) < 0)

FAIL("waitpid");

TEST_ASSERT(WIFEXITED(status));

TEST_ASSERT(WEXITSTATUS(status) == 0);

if (stat_files(sfd, sfdcnt, 0) < 0)

FAIL("stat_files");

if (unlink_files(sfdcnt) < 0)

FAIL("unlink_files");

close(fd[0]);

close(fd[1]);

}

else {

int rfd[MAX_FD_CNT];

ssize_t n;

Yes, this was used in the past. This code is pushed now and it should conform to current coding convention.

@nalajcie Could you give your opinion on that?

@adamdebek still the variable names are not correct, please use camelCase.

damianloew · 2023-08-09T14:54:15Z

exit/test_exit.c

+		atexit(test_fun1);
+		atexit(test_fun1);


Is calling it 2 times intentional? If so, please leave a comment.

damianloew · 2023-08-09T14:56:51Z

exit/test_exit.c

+		for (i = 0; i < atexit_max - 1; i++) {
+			TEST_ASSERT_EQUAL_INT(0, atexit(fun1));
+		}
+		TEST_ASSERT_EQUAL_INT(0, atexit(fun2));
+		TEST_ASSERT_EQUAL_INT(0, atexit(fun1));


Is this order asserted somewhere?

damianloew

For now looked through _exit() tests and a lot of requirements from the documentation are not covered. I understand that 100% coverage is not possible, though there is certainly a lot of test cases to create. I recommend you some method for verifying what is already covered, for example:

Please leave a comment what was not covered and why (when it's impossible).

However, the code is really clean and understandable.

exit/Makefile

exit/test.yaml

exit/test_exit.c

github-actions · 2023-09-09T18:08:28Z

Unit Test Results

5 884 tests +150 5 233 ✔️ +117 29m 10s ⏱️ +34s
  316 suites +    5   651 💤 +  33
      1 files ±    0       0 ❌ ±    0

Results for commit 2cd4b47. ± Comparison against base commit dfa5bde.

♻️ This comment has been updated with latest results.

damianloew

EDIT: As you mentioned - XSI requirements are not required in POSIX

exit/test.yaml

exit/test_exit.c

damianloew · 2023-09-18T15:04:33Z

exit/test_exit.c

+		  process group is stopped, then a SIGHUP signal followed by a SIGCONT signal shall be sent to each process in
+		  the newly-orphaned process group -> https://github.com/phoenix-rtos/phoenix-rtos-project/issues/809
+ */
+


the full value shall be available from waitid() and in the siginfo_t passed to a signal handler for SIGCHLD. - not covered.

From what I see

...in the siginfo_t passed to a signal handler for SIGCHLD - it's possible to check - you probably can get siginfo_t in the handler function

the full value shall be available from waitid() - temporary not possible to check, due to lack of waitid() func - please leave a comment about it.

siginfo_t can be accessed only when SA_SIGINFO flag is set, but it is not present in PHOENIX-RTOS right now.

Yes, waitid() not implemented.

damianloew · 2023-09-18T15:08:37Z

exit/test_exit.c

+		  process group is stopped, then a SIGHUP signal followed by a SIGCONT signal shall be sent to each process in
+		  the newly-orphaned process group -> https://github.com/phoenix-rtos/phoenix-rtos-project/issues/809
+ */
+


The _Exit() and _exit() functions shall be functionally equivalent. - also not covered - please add at least one twin case with _Exit call instead of _exit().

exit/test_exit.c

damianloew · 2023-09-18T16:09:41Z

exit/test_exit.c

+		  process group is stopped, then a SIGHUP signal followed by a SIGCONT signal shall be sent to each process in
+		  the newly-orphaned process group -> https://github.com/phoenix-rtos/phoenix-rtos-project/issues/809
+ */
+


directory streams, conversion descriptors, and message catalog descriptors open in the calling process shall be closed - I think director streams (*DIR) may be asserted, I am not sure about conversion descriptors, and message catalog descriptors - to be checked. If sth is not possible please leave a comment.

AFAIK it is not possible to check closure of directory stream.

exit/test_exit.c

damianloew · 2023-09-18T16:33:04Z

exit/test_exit.c

+		  process group is stopped, then a SIGHUP signal followed by a SIGCONT signal shall be sent to each process in
+		  the newly-orphaned process group -> https://github.com/phoenix-rtos/phoenix-rtos-project/issues/809
+ */
+


And the rest of points - I haven't verified the rest yet.

damianloew

Some ideas for more cases + suggested information about not covered requirements

exit/test_exit.c

exit/test-exit.c

damianloew · 2023-10-02T14:21:59Z

exit/test-exit.c

+				if (argsThr2.retWaitThr == -1 && argsThr2.errnoThr == ECHILD) {
+					TEST_PASS();
+				}
+				else if (argsThr2.retWaitThr != 0 && argsThr2.errnoThr == 0) {
+					TEST_FAIL_MESSAGE("Error: More than 1 thread unblocked");
+				}


What if different errno happened?

Suggested change

if (argsThr2.retWaitThr == -1 && argsThr2.errnoThr == ECHILD) {

TEST_PASS();

}

else if (argsThr2.retWaitThr != 0 && argsThr2.errnoThr == 0) {

TEST_FAIL_MESSAGE("Error: More than 1 thread unblocked");

}

if (argsThr2.retWaitThr == -1) {

if (argsThr2.errnoThr == ECHILD) {

TEST_PASS();

} else {

TEST_FAIL_MESSAGE("Error: wait() failed with an invalid errno: %d" argsThr2.errnoThr);

}

}

else if (argsThr2.retWaitThr != -1 && argsThr2.errnoThr == 0) {

TEST_FAIL_MESSAGE("Error: More than 1 thread unblocked");

}

There is only 1 more errno value, which is EINTR but that is impossible to happen.

Even if it's hardly possible I'd like to handle it (the potentially may be some issue with a system).

exit/test-exit.c

damianloew · 2023-10-02T15:18:40Z

exit/test-exit.c

+}
+
+
+TEST(unistd_exit, status_vals)


such case missing for stdlib exit

Stdlib exit calls _exit() from unistd.

It potentially may change.

It will not change, these function are designed to be called in this manner.

exit/test-exit.c

libc/exit/exit.c

JIRA: CI-209

damianloew

Thanks for all the corrections, LGTM

adamdebek marked this pull request as draft May 23, 2023 11:27

adamdebek force-pushed the adamdebek/exit_test branch from 140aa06 to b99df3c Compare May 23, 2023 17:12

github-actions bot reviewed May 23, 2023

View reviewed changes

libc/exit.c Outdated Show resolved Hide resolved

adamdebek force-pushed the adamdebek/exit_test branch 2 times, most recently from b34af1c to 42153c6 Compare May 29, 2023 13:49

adamdebek force-pushed the adamdebek/exit_test branch from 42153c6 to a4736b2 Compare June 12, 2023 13:05

adamdebek marked this pull request as ready for review June 13, 2023 11:17

adamdebek force-pushed the adamdebek/exit_test branch 2 times, most recently from b1ab1d4 to 91f4107 Compare June 22, 2023 13:42

niewim19 requested changes Jun 22, 2023

View reviewed changes

damianloew reviewed Jun 22, 2023

View reviewed changes

exit/test_exit.c Outdated Show resolved Hide resolved

adamdebek force-pushed the adamdebek/exit_test branch 2 times, most recently from f149842 to 9c1506a Compare July 5, 2023 15:39

github-actions bot reviewed Jul 5, 2023

View reviewed changes

exit/test_exit.c Outdated Show resolved Hide resolved

adamdebek force-pushed the adamdebek/exit_test branch from 9c1506a to 6717dc1 Compare July 5, 2023 20:20

adamdebek requested a review from niewim19 July 6, 2023 09:56

niewim19 requested changes Jul 14, 2023

View reviewed changes

adamdebek force-pushed the adamdebek/exit_test branch from 6717dc1 to 5411388 Compare July 14, 2023 14:59

adamdebek requested review from damianloew and niewim19 July 17, 2023 10:07

damianloew reviewed Aug 9, 2023

View reviewed changes

damianloew requested changes Aug 10, 2023

View reviewed changes

adamdebek force-pushed the adamdebek/exit_test branch from 5411388 to bfc9968 Compare September 9, 2023 17:54

github-actions bot reviewed Sep 9, 2023

View reviewed changes

exit/test_exit.c Outdated Show resolved Hide resolved

adamdebek force-pushed the adamdebek/exit_test branch from bfc9968 to d6c771f Compare September 9, 2023 17:58

github-actions bot reviewed Sep 9, 2023

View reviewed changes

exit/test_exit.c Outdated Show resolved Hide resolved

adamdebek force-pushed the adamdebek/exit_test branch from d6c771f to cbebc7e Compare September 11, 2023 10:11

adamdebek requested a review from damianloew September 11, 2023 10:26

damianloew requested changes Sep 18, 2023

View reviewed changes

adamdebek force-pushed the adamdebek/exit_test branch from cbebc7e to 25b68f3 Compare September 19, 2023 14:16

adamdebek force-pushed the adamdebek/exit_test branch from 25b68f3 to 926247c Compare September 29, 2023 07:19

damianloew requested changes Sep 29, 2023

View reviewed changes

adamdebek force-pushed the adamdebek/exit_test branch from 926247c to ad0ad38 Compare September 30, 2023 14:46

github-actions bot reviewed Sep 30, 2023

View reviewed changes

exit/test_exit.c Outdated Show resolved Hide resolved

adamdebek force-pushed the adamdebek/exit_test branch from ad0ad38 to 63bda5b Compare September 30, 2023 17:44

github-actions bot reviewed Sep 30, 2023

View reviewed changes

exit/test-exit.c Outdated Show resolved Hide resolved

damianloew requested changes Oct 2, 2023

View reviewed changes

adamdebek force-pushed the adamdebek/exit_test branch 5 times, most recently from 37e48a7 to e39c937 Compare October 4, 2023 14:42

adamdebek force-pushed the adamdebek/exit_test branch from e39c937 to 41222c3 Compare October 11, 2023 14:37

github-actions bot reviewed Oct 11, 2023

View reviewed changes

libc/exit/exit.c Outdated Show resolved Hide resolved

libc/exit/exit.c Outdated Show resolved Hide resolved

adamdebek force-pushed the adamdebek/exit_test branch 3 times, most recently from a641473 to 9ffd0d3 Compare October 11, 2023 14:55

adamdebek requested a review from damianloew October 11, 2023 15:11

adamdebek force-pushed the adamdebek/exit_test branch 2 times, most recently from 8f6ea7e to cf9ebf5 Compare October 12, 2023 14:06

damianloew requested changes Oct 12, 2023

View reviewed changes

adamdebek force-pushed the adamdebek/exit_test branch from cf9ebf5 to f24a1c6 Compare October 13, 2023 11:24

adamdebek requested a review from damianloew October 13, 2023 12:12

libc: add exit, _exit, atexit tests

2cd4b47

JIRA: CI-209

adamdebek force-pushed the adamdebek/exit_test branch from f24a1c6 to 2cd4b47 Compare October 13, 2023 14:55

damianloew approved these changes Oct 13, 2023

View reviewed changes

niewim19 approved these changes Oct 15, 2023

View reviewed changes

adamdebek merged commit de54a95 into master Oct 16, 2023

adamdebek deleted the adamdebek/exit_test branch October 16, 2023 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adamdebek/exit test #208

Adamdebek/exit test #208

adamdebek commented May 23, 2023 •

edited

Loading

niewim19 left a comment •

edited

Loading

niewim19 Jun 22, 2023

adamdebek Jul 5, 2023

niewim19 Jun 22, 2023

adamdebek Jul 5, 2023 •

edited

Loading

damianloew Aug 9, 2023

niewim19 Jul 14, 2023

adamdebek Jul 14, 2023

niewim19 Jul 17, 2023

adamdebek Jul 31, 2023

damianloew Aug 9, 2023

damianloew Aug 9, 2023

damianloew Aug 9, 2023

damianloew left a comment •

edited

Loading

github-actions bot commented Sep 9, 2023 •

edited

Loading

damianloew left a comment

damianloew Sep 18, 2023

adamdebek Sep 19, 2023

adamdebek Sep 19, 2023 •

edited

Loading

damianloew Sep 18, 2023

damianloew Sep 18, 2023

adamdebek Sep 19, 2023

damianloew Sep 18, 2023

damianloew left a comment

damianloew Oct 2, 2023

adamdebek Oct 2, 2023

damianloew Oct 12, 2023 •

edited by adamdebek

Loading

damianloew Oct 2, 2023

adamdebek Oct 2, 2023

damianloew Oct 12, 2023

adamdebek Oct 12, 2023

damianloew left a comment

	int fd[2];
	pid_t pid;
	size_t sfdcnt, rfdcnt;

	sfdcnt = rand() % (MAX_FD_CNT + 1);

	if (socketpair(AF_UNIX, type, 0, fd) < 0)
	FAIL("socketpair");

	if ((pid = fork()) < 0)
	FAIL("fork");

	if (pid) {
	int sfd[MAX_FD_CNT];
	ssize_t n;
	int status;

	if (open_files(sfd, sfdcnt) < 0)
	FAIL("open_files");

	if (write_files(sfd, sfdcnt) < 0)
	FAIL("write_files");

	n = unix_msg_send(fd[0], data, 1, sfd, sfdcnt);
	TEST_ASSERT(n == 1);

	if (close_files(sfd, sfdcnt) < 0)
	FAIL("close_files");

	if (waitpid(pid, &status, 0) < 0)
	FAIL("waitpid");

	TEST_ASSERT(WIFEXITED(status));
	TEST_ASSERT(WEXITSTATUS(status) == 0);

	if (stat_files(sfd, sfdcnt, 0) < 0)
	FAIL("stat_files");

	if (unlink_files(sfdcnt) < 0)
	FAIL("unlink_files");

	close(fd[0]);
	close(fd[1]);
	}
	else {
	int rfd[MAX_FD_CNT];
	ssize_t n;

		}


		TEST(unistd_exit, status_vals)

Adamdebek/exit test #208

Adamdebek/exit test #208

Conversation

adamdebek commented May 23, 2023 • edited Loading

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

niewim19 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdebek Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianloew left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 9, 2023 • edited Loading

Unit Test Results

damianloew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamdebek Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianloew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianloew Oct 12, 2023 • edited by adamdebek Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianloew left a comment

Choose a reason for hiding this comment

adamdebek commented May 23, 2023 •

edited

Loading

niewim19 left a comment •

edited

Loading

adamdebek Jul 5, 2023 •

edited

Loading

damianloew left a comment •

edited

Loading

github-actions bot commented Sep 9, 2023 •

edited

Loading

adamdebek Sep 19, 2023 •

edited

Loading

damianloew Oct 12, 2023 •

edited by adamdebek

Loading