Arkadiusz kozlowski/opendir tests #278

KArkadiusz · 2023-10-19T13:46:35Z

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.

damianloew

I've left some improvements and I've indicated some lacks in the coverage of requirements. However the existent code looks clean, and I like its structure.

damianloew · 2023-10-19T16:30:13Z

libc/Makefile

@@ -23,3 +23,4 @@ $(eval $(call add_test_libc,misc))
 $(eval $(call add_test_libc,stdio))
 $(eval $(call add_test_libc,stdlib))
 $(eval $(call add_test_libc,string))
+$(eval $(call add_test_libc,dirent))


Please always add newline at the end of file. Unless you do it, there will be this red sign.

damianloew · 2023-10-19T16:32:10Z

libc/dirent/main.c

+ * Copyright 2021, 2022 Phoenix Systems
+ * Author: Marek Bialowas, Damian Loewnau


You can assume that you are the only author in this case. Moreover please correct the date.

damianloew · 2023-10-20T11:25:51Z

libc/dirent/opendir.c

+}
+
+
+TEST(opendir, opening_not_empty_directory)


This case is not called.

damianloew · 2023-10-20T11:29:11Z

libc/dirent/opendir.c

+TEST_GROUP(opendir);
+
+
+TEST_SETUP(opendir)


I would opt for leaving only the things necessary for all the cases in setup/tear_down functions as they are called before/after every test case.

Applies to the rest of tests.

damianloew · 2023-10-20T11:30:05Z

libc/dirent/opendir.c

+}
+
+
+TEST(opendir, opening_not_empty_directory)


Creating case, where empty directory is checked will be a good idea.

damianloew · 2023-10-24T10:45:55Z

libc/dirent/readdir.c

+TEST_GROUP_RUNNER(readdir)
+{
+	RUN_TEST_CASE(readdir, basic_listing_count);
+	RUN_TEST_CASE(readdir, correct_dirent_names);


What about testing errnos in this test? I am not sure about EOVERFLOW (maybe if d_name was too long) and ENOENT (don't have idea for reproducing it for now), but EBADF seems to be simple to check. Please leave a comment if some requirement is not possilble to test using our current testing environment.

damianloew · 2023-10-24T10:54:32Z

libc/dirent/opendir.c

+	RUN_TEST_CASE(opendir, not_a_directory);
+	RUN_TEST_CASE(opendir, direct_symlink);
+	RUN_TEST_CASE(opendir, too_long_path);
+	RUN_TEST_CASE(opendir, open_n_directories);


You can also add a case, where you:

open directory with some initial content

read the whole content and assert it's proper

close the directory

create some additional files within mentioned dir

open it for a second time

check whether the content has been updated

It shall also cause the directory stream to refer to the current state of the corresponding directory, as a call to opendir()would have done - rewinddir docs. So you can also add the analogical case in rewinddir tests.

I'm not sure how I would read the whole content from no. 2, in opendir at most I can check if I have access to newly created files, even if the dir isn't opened.

damianloew · 2023-10-24T10:57:31Z

libc/dirent/rewinddir.c

+TEST_GROUP_RUNNER(rewinddir)
+{
+
+	RUN_TEST_CASE(rewinddir, reset_dirstream_position);


After a call to the fork() function, either the parent or child (but not both) may continue processing the directory stream using rewinddir(), - please try to call rewinddir in a child.

damianloew · 2023-10-24T10:59:14Z

libc/dirent/rewinddir.c

+	while (readdir(dp))
+		counter3++;
+
+	TEST_ASSERT_NOT_EQUAL(counter1, counter3);


I'd opt for asserting the exact value, so in this case counter3 to be 3.

damianloew · 2023-10-24T11:02:42Z

libc/dirent/closedir.c

+		}
+	}
+
+	kill(childID, SIGKILL);


SIGKILL also kills the parent - that's why it won't work. I am not sure whether this requirement is valid on our system and possible to test - to be discussed. Notice that it's not strictly required on POSIX as word may is used: The closedir() function may fail if:. For now you can leave those cases commented out.

github-actions · 2023-10-24T12:00:08Z

Unit Test Results

6 167 tests +178 5 500 ✔️ +152 34m 17s ⏱️ + 4m 50s
  341 suites +    8   657 💤 +  16
      1 files ±    0     10 ❌ +  10

For more details on these failures, see this check.

Results for commit c0f421a. ± Comparison against base commit 36b0b78.

♻️ This comment has been updated with latest results.

libc/dirent/opendir.c

damianloew requested changes Oct 24, 2023

View reviewed changes

KArkadiusz force-pushed the ArkadiuszKozlowski/opendirTests branch from c3e930a to 74bd017 Compare October 24, 2023 11:53

KArkadiusz requested a review from damianloew November 2, 2023 17:15

mateusz-bloch reviewed Nov 6, 2023

View reviewed changes

libc/dirent/opendir.c Outdated Show resolved Hide resolved

KArkadiusz force-pushed the ArkadiuszKozlowski/opendirTests branch from 46c7ebf to e1207ea Compare November 8, 2023 16:39

KArkadiusz requested a review from mateusz-bloch November 9, 2023 14:44

damianloew requested a review from adamdebek November 10, 2023 11:00

#change me

c0f421a

KArkadiusz force-pushed the ArkadiuszKozlowski/opendirTests branch from bd78a18 to c0f421a Compare November 10, 2023 11:22

KArkadiusz closed this Nov 13, 2023

KArkadiusz deleted the ArkadiuszKozlowski/opendirTests branch November 13, 2023 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arkadiusz kozlowski/opendir tests #278

Arkadiusz kozlowski/opendir tests #278

KArkadiusz commented Oct 19, 2023

damianloew left a comment

damianloew Oct 19, 2023

damianloew Oct 19, 2023

damianloew Oct 20, 2023

damianloew Oct 20, 2023

damianloew Oct 23, 2023

damianloew Oct 20, 2023

damianloew Oct 24, 2023

damianloew Oct 24, 2023

KArkadiusz Oct 24, 2023

damianloew Oct 24, 2023

damianloew Oct 24, 2023

damianloew Oct 24, 2023

github-actions bot commented Oct 24, 2023 •

edited

Loading

		* Copyright 2021, 2022 Phoenix Systems
		* Author: Marek Bialowas, Damian Loewnau

		}


		TEST(opendir, opening_not_empty_directory)

		TEST_GROUP(opendir);


		TEST_SETUP(opendir)

		}


		TEST(opendir, opening_not_empty_directory)

Arkadiusz kozlowski/opendir tests #278

Arkadiusz kozlowski/opendir tests #278

Conversation

KArkadiusz commented Oct 19, 2023

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

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

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

Choose a reason for hiding this comment

github-actions bot commented Oct 24, 2023 • edited Loading

Unit Test Results

github-actions bot commented Oct 24, 2023 •

edited

Loading