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

Run passing tests from pjdfstest #1882

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

gaul
Copy link
Member

@gaul gaul commented Jan 30, 2022

This downloads a tarball by hash instead of using a submodule.
References #1589.

@gaul gaul requested a review from ggtakec January 30, 2022 10:27
autogen.sh Outdated
@@ -38,6 +38,8 @@ aclocal \
&& automake --add-missing \
&& autoconf

(cd test/pjdfstest && autoreconf -ifs && ./configure)
Copy link
Member Author

Choose a reason for hiding this comment

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

Any ideas on what is the correct way to do this?

Copy link
Member

Choose a reason for hiding this comment

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

Setting pjdfstest as a submodule is a bit thoughtful.

I want the s3fs repository to be able to work independently.
I don't think I agree with mixing the pjdfstest build with the s3fs build.
The pjdfstest build is the environment required for the s3fs build(and test), and I think it's better to separate this part.

If you want to use pjdfstest in s3fs test, what about the following method?

  • In github actions, git clone pjdfstest under the test directory(this can be done with ci.yml. You can prepare a helper script if necessary)
  • And in the pjdfstest directory, do that build

How about performing this step only during CI?

Anyone can deploy it on localhost other than CI by preparing a dedicated helper script.
(Alternatively, you may prepare another repository(ex. s3fs-fuse/s3fs-pjdfstest, etc.) and dedicate it. But this is still an idea I just come up with, so it may be wrong.)

Copy link
Member

Choose a reason for hiding this comment

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

I came up with another one.
You may want to call the script as a PHONY target in the Makefile (.am) of the test directory.
(Clone and build pjdfstest in the script)

Copy link
Member

Choose a reason for hiding this comment

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

I made a example. (see proto_pjdfstest branch in my personal repo)
ggtakec/s3fs-fuse@master...proto_pjdfstest

I haven't implemented a "test" using pjdfstest yet...(because I don't know how you want to do it)
This way, pjdfstest can be made independent for now, which is in line with my wishes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reworked this to download the tarball which is cleaner.

@gaul gaul force-pushed the pjdfstest branch 5 times, most recently from 804e924 to d86ca1a Compare August 27, 2023 23:04
@gaul gaul changed the title Run passing utimensat tests from pjdfstest Run passing tests from pjdfstest Aug 27, 2023
@gaul
Copy link
Member Author

gaul commented Aug 27, 2023

This seems to show some real failure with rmdir and -o use_cache

@gaul gaul marked this pull request as ready for review August 28, 2023 18:04
@gaul
Copy link
Member Author

gaul commented Aug 28, 2023

This increases CI run-time by 50% so we should be careful if we want to merge this PR.

This downloads a tarball by hash instead of using a submodule.
References s3fs-fuse#1589.
@gaul
Copy link
Member Author

gaul commented Sep 14, 2023

CI recently started failing with these kinds of errors:

2023-09-14T19:14:38.4260487Z cp(-p) expected times: mtime( 1694718792.836254751 == 1694718792.852005368 ), ctime( 1694718792.836254751 != 1694718792.848255678 ), atime( 1694718792.834613324 == 1694718792.834613324 )
2023-09-14T19:14:38.4260909Z test_mtime_file failed

2023-09-14T19:14:38.4362592Z cp with -p option expected updated ctime: 1000000000.000000000 != 1694718798.132663109 and same mtime: 1000000000.000000000 == 1000000000.000000000, atime: 1000000000.000000000 == 1694718798.133826699
2023-09-14T19:14:38.4363207Z test_update_time_cp_p failed

I don't think they are related to this PR.

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.

None yet

2 participants