Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

K arkadiusz/dirent test #294

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

K arkadiusz/dirent test #294

wants to merge 2 commits into from

Conversation

KArkadiusz
Copy link

@KArkadiusz KArkadiusz commented Nov 14, 2023

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.

Copy link

github-actions bot commented Nov 14, 2023

Unit Test Results

6 591 tests  +181   5 946 ✅ +170   33m 12s ⏱️ +25s
  362 suites +  8     642 💤 +  8 
    1 files   ±  0       3 ❌ +  3 

For more details on these failures, see this check.

Results for commit ad63326. ± Comparison against base commit 5d4aee2.

♻️ This comment has been updated with latest results.

libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/main.c Outdated Show resolved Hide resolved
libc/dirent/main.c Outdated Show resolved Hide resolved
libc/dirent/main.c Outdated Show resolved Hide resolved
libc/dirent/closedir.c Outdated Show resolved Hide resolved
libc/dirent/closedir.c Outdated Show resolved Hide resolved
libc/dirent/closedir.c Outdated Show resolved Hide resolved
libc/dirent/closedir.c Outdated Show resolved Hide resolved
libc/dirent/closedir.c Outdated Show resolved Hide resolved
libc/dirent/closedir.c Outdated Show resolved Hide resolved
libc/dirent/closedir.c Outdated Show resolved Hide resolved
libc/dirent/closedir.c Outdated Show resolved Hide resolved
libc/dirent/rewinddir.c Outdated Show resolved Hide resolved
libc/dirent/rewinddir.c Outdated Show resolved Hide resolved
libc/dirent/rewinddir.c Outdated Show resolved Hide resolved
libc/dirent/rewinddir.c Outdated Show resolved Hide resolved
libc/dirent/rewinddir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/opendir.c Outdated Show resolved Hide resolved
libc/dirent/opendir.c Outdated Show resolved Hide resolved
libc/dirent/opendir.c Outdated Show resolved Hide resolved
libc/dirent/opendir.c Outdated Show resolved Hide resolved
libc/dirent/opendir.c Outdated Show resolved Hide resolved
libc/dirent/opendir.c Outdated Show resolved Hide resolved
libc/dirent/opendir.c Outdated Show resolved Hide resolved
Copy link
Contributor

@adamdebek adamdebek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some common things across the tests, which I point here:

  • Delete not used headers
  • Use macros instead wraper function to check errors, we want to see line number where error occurred
  • Use typed asserts instead of TEST_ASSERT_EQUAL
  • Use minimal needed permissions not 0777 everywhere
  • Declare variables at the start of block not in the middle
  • Some functions from your helper header are used only once so it misses the purpose of placing them in headers
  • Use curly braces with conditional statements (many are missing)
  • Many functions miss check for returned value

libc/dirent/closedir.c Outdated Show resolved Hide resolved
libc/dirent/closedir.c Show resolved Hide resolved
libc/dirent/opendir.c Outdated Show resolved Hide resolved
libc/dirent/opendir.c Outdated Show resolved Hide resolved
libc/dirent/opendir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/readdir.c Show resolved Hide resolved
libc/dirent/readdir.c Outdated Show resolved Hide resolved
libc/dirent/opendir.c Outdated Show resolved Hide resolved
KArkadiusz added 2 commits January 26, 2024 21:02
dirent: applied some suggestions
JIRA: CI-359
libc/Makefile Show resolved Hide resolved
libc/dirent/dirent_helper_functions.h Show resolved Hide resolved
libc/dirent/dirent_helper_functions.h Show resolved Hide resolved
@@ -0,0 +1,19 @@
#ifndef _DIRENT_HELPER_FUNCTIONS_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it shouldn't be placed in common header of whole libc.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they'll ever be needed anywhere else.

libc/dirent/readdir.c Show resolved Hide resolved
}


TEST(dirent_readdir, long_name_directory_check)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this testcase fails on some targets so it should be ignored for now and appropriate issue reported. See also that NAME_MAX can be variable depending on path and pathconf could be used to determine it, however pathconf is not implemented at this moment.

libc/dirent/readdir.c Show resolved Hide resolved
libc/dirent/readdir.c Show resolved Hide resolved
libc/dirent/readdir.c Show resolved Hide resolved
libc/dirent/readdir.c Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants