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

libc: unittests for path resolving (realpath(), canonicalize_file_name(), etc.) #29

Merged
merged 3 commits into from
Jul 8, 2021

Conversation

nalajcie
Copy link
Member

@nalajcie nalajcie commented Jun 23, 2021

Description

libc unit test framework stub + tests around pathname resolution.

The tests will fail until all relevant changes in kernel / filesystems / libphoenix will be merged

Motivation and Context

Rewriting path handling in libphoenix.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

  • New test added: this PR

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.

This PR is to be merged only after all other relevant PR will be merged and CI checks for this test would pass

I would appreciate review anyway.

libc/resolve_path.c Outdated Show resolved Hide resolved
@nalajcie
Copy link
Member Author

@Maxez @damianloew @niewim19 : no need for full code review, just showing how tests could/should be written.

@nalajcie nalajcie force-pushed the nalajcie/realpath branch from e063593 to 65dffc3 Compare June 24, 2021 11:15
@gerard5
Copy link
Contributor

gerard5 commented Jun 25, 2021

Nothing is mentioned in PR, which platforms these tests apply to, so I decided to check on imxrt1064 due to problems with canonicalize_file_name() in psh and in other places.

These tests are valid only for real root-fs as I can see (/etc, /etc/passwd and other files need to exist to pass the test), I compiled test-libc with Your recent changes (not yet merged) and launched on imxrt1064 with dummyfs, I manually created dirs and files the test-libc expects, and it failed (on /etc/passwd/fake which of course does not exist due to /etc/passwd is a file not directory, the test should pass here ENOTDIR, instead it fails loudly, possibly due to stack overflow):

TEST(resolve_path, canonicalize_abs_simple) PASS                                                                                                                                                                                               
TEST(resolve_path, canonicalize_pwd_simple) PASS                                                                                                                                                                                               
TEST(resolve_path, realpath_abs_noalloc) PASS                                                                                                                                                                                                  
TEST(resolve_path, realpath_pwd_noalloc) PASS                                                                                                                                                                                                  
ASSERTION /home/gerard/projects/rt1064/snarch/phoenix-rtos-tests/libc/resolve_path.c:154:FAIL: Expected NULL                                                                                                                
TEST(resolve_path, realpath_errno) FAIL at /home/gerard/projects/rt1064/snarch/phoenix-rtos-tests/libc/resolve_path.c:154                                                                                                   
Exception: 6 #UsageFault                                                                                                                                                                                                                       
 r0=00000002  r1=00000005  r2=00000004  r3=00000000                                                                                                                                                                                            
 r4=00000000  r5=20221c18  r6=20220e7c  r7=20221c18                                                                                                                                                                                            
 r8=00000000  r9=20220948 r10=00000000 r11=00000000                                                                                                                                                                                            
r12=00000000  sp=20017794  lr=00000003  pc=00000000                                                                                                                                                                                            
psp=20220d98 psr=00000000 exr=ffffffed bfa=00000000                                                                                                                                                                                            
cfs=00020000                                                                                                                                                                                                                                   

@nalajcie
Copy link
Member Author

Nothing is mentioned in PR, which platforms these tests apply to, so I decided to check on imxrt1064 due to problems with canonicalize_file_name() in psh and in other places.

Thanks for checking. The Exception is probably due to setjmp/longjmp not working on rt1064 (which is a blocker for automatic testing on rt1064). You can change unity implementation not to use longjmp, then the tests should work.

Regarding the failed test - it is probably an issue with dummyfs implementation (I've only checked the tests with /tmp prefix to work on dummyfs; unfortunately testing dummyfs on ia32-generic is not possible due to phoenix-rtos/phoenix-rtos-project#114 and I don't have any HW on hand to test it the other way).

Please verify if You have the most recent phoenix-rtos-filesystems repo as there were some fixes to various filesystems done there recently (due to issues found running these tests).

@gerard5
Copy link
Contributor

gerard5 commented Jun 25, 2021

Please verify if You have the most recent phoenix-rtos-filesystems repo as there were some fixes to various filesystems done there recently (due to issues found running these tests).

Yes, I have all recent master versions (if Your branch is not present in repo) and if it is present than Your branch is checked out and HEAD points SHA from Your latest commit, like e.g. for libphoenix and unity of course with UNITY_EXCLUDE_SETJMP defined in unity_config.h so setjmp is not used, test fails as above.

I'm executing the test form syspage using command like sysexec ocram2 test-libc, I tried dtcm map - no change.

Copy link
Contributor

@jsarzy jsarzy left a comment

Choose a reason for hiding this comment

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

I verified test on ia32-generic. I also ran test on imxrt106x with fixed setjmp:

  • I got same error as @gerard5 (but without exception)
  • I got exception (I think) on check_file_contents

Overall tests look good, without taking into account some issues with cleaning.

I don't know if I'm big fan of one binary that tests the whole libc:

  • We can't exclude some architectures from individual tests
  • As you mentioned in comments it would be great to pass some arguments (like a path prefix) to the test binary. With one binary it won't be convenient.
  • With separate binaries we have clean state at the start of test
  • Using runner as is we can't see progress of individual tests

On the other hand, executing one binary will be much faster ;).

libc/common.h Outdated Show resolved Hide resolved
libc/resolve_path.c Show resolved Hide resolved
@nalajcie
Copy link
Member Author

nalajcie commented Jul 8, 2021

Thanks for checking, my answers below

I verified test on ia32-generic. I also ran test on imxrt106x with fixed setjmp:

  • I got same error as @gerard5 (but without exception)

dummyfs error as suspected, should be fixed by: phoenix-rtos/phoenix-rtos-filesystems#42

  • I got exception (I think) on check_file_contents

I suppose this might be a stack overflow than, will reduce the _buf size to 128 (I don't have actual HW to test it).

Overall tests look good, without taking into account some issues with cleaning.

Written as a todo, would be nice to have recursive rm to clean full workdir between the testaceses (as a part of TEST_SETUP and TEST_TEAR_DOWN)

I don't know if I'm big fan of one binary that tests the whole libc:

  • We can't exclude some architectures from individual tests

My idea was that if it would be necessary, we could define separate YAML tests as the same binary with different test groups being run (we control how main works, so we may introduce test group wildcards as necessary, etc.). Then running multiple tests from the same binary might be optimized just in runner.py implementation.

I believe most of the tests should work on most architectures (even if not now than in the future).

In case of my current test - it seems it could be done with some preparation (mounting dummyfs and touch /etc/passwd, we might want to think about a way to do just that automatically somehow).

  • As you mentioned in comments it would be great to pass some arguments (like a path prefix) to the test binary. With one binary it won't be convenient.

No, we just need to implement some sane way of passing the arguments. I believe we should have named params (key=value) available in some global context (with sane access methods) - but that's an improvement for the future.

  • With separate binaries we have clean state at the start of test

No, not with separate binaries but with restarts between the tests - and only considering readonly rootfs (which is not the case on ia32-generc) - so even having separate binaries doesn't guarantee the

  • Using runner as is we can't see progress of individual tests

Oh, this could be solved in unittest harness (and might be actually a good thing to be done :)).

On the other hand, executing one binary will be much faster ;).

Yeah, my initial and most strong motivation. The next one is that there are many repetitive tasks when writing libc tests (but that could be made a separate lib linked against many binaries, so not a definite argument for now).

@nalajcie nalajcie force-pushed the nalajcie/realpath branch from 65dffc3 to ef19886 Compare July 8, 2021 08:00
@jsarzy
Copy link
Contributor

jsarzy commented Jul 8, 2021

In case of my current test - it seems it could be done with some preparation (mounting dummyfs and touch /etc/passwd, we might want to think about a way to do just that automatically somehow).

In the "pure" harness test it could be done by using some setup and teardown functions. It also could work for unit test, just add some setup.py with these functions. It will be more difficult for single groups of unit test but still feasible.

Oh, this could be solved in unittest harness (and might be actually a good thing to be done :)).

Yeap, it should be not hard to do. I'm going to create issues what can be done to improve runner so it'll be listed :)

I re-ran tests on imxrt106x:
image

@nalajcie
Copy link
Member Author

nalajcie commented Jul 8, 2021

@jsarzy @gerard5 as phoenix-rtos/libphoenix#99 is now merged and upstreamed, can You give an approval so I can merge this.

Thanks for checking on rt1064, we found another bug in dummyfs thanks to that.

I hope we will be able to automatically run this test on rt1064 also, but that should be a topic of separate PR.

@nalajcie nalajcie requested review from jsarzy and gerard5 July 8, 2021 10:51
@gerard5
Copy link
Contributor

gerard5 commented Jul 8, 2021

I've pulled all recent submodules
realpath-012cd443

@nalajcie
Copy link
Member Author

nalajcie commented Jul 8, 2021

@gerard5 seems like You're using old toolchain thus including invalid limits.h in this source file.

@nalajcie nalajcie merged commit e88d0c7 into master Jul 8, 2021
@nalajcie nalajcie deleted the nalajcie/realpath branch July 8, 2021 11:12
@gerard5
Copy link
Contributor

gerard5 commented Jul 8, 2021

@gerard5 seems like You're using old toolchain thus including invalid limits.h in this source file.

Yes you are right my mistake. I am working on my home computer today and forgot that the limits.h change recently was made, now I rebuilt the toolchain and it worked.
Closing the topic, and to confirm that I also ran the test successfully, here's the screenshot:
realpath-a588bdf43

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