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

Mark requirements #56

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Mark requirements #56

wants to merge 24 commits into from

Conversation

twied
Copy link
Contributor

@twied twied commented Jan 30, 2022

Introduces two new functions, "push_requirement" and "pop_requirement", that govern a stack of current requirements. If the requirements are unmet, tests are skipped. This has several positive effects:

  • Allows to run as non-root the subset of pjdfstests that work as non-root
  • Allows to selectively disable tests for certain file types (closes Excluding file types #45)
  • Simplifies test plan calculation ("1..n" lines)

While at it, this series also fixes a bug in rename/09.t and rename/10.t and provides more information in the output (not quite fixing #27, but certainly helping).

@ngie-eign
Copy link
Collaborator

ngie-eign commented Jan 30, 2022

I really appreciate all of the efforts you put in.. but I’m going to softly decline your proposal.
I have a proposal I’m going to put out to @asomers / @pjd soon to move away from expressing the tests in TAP to pytest.

pytest a much more expressive/pluggable test infrastructure that avoids many of the shortcomings you’re identifying with testcase indexing and the like, and does M x N style testing with fixtures much more gracefully than the adhoc setup and tear down logic in pkdfstest does today.

Moving to pytest for the test infrastructure would allow pjdfstest to focus more on what needs to be tested, instead of how it’s being tested.

@twied
Copy link
Contributor Author

twied commented Feb 1, 2022

Rewriting the tests in Python sounds great!

I believe that the MR would actually help in that endeavor. The patches add (or rather: make explicit) the list of requirements for the tests but do not alter the structure of the test files. That should be handy information to have and not get in the way of any work already done.

Any idea how long the rewrite will take? I can see the merge request from March last year, but no activity after the first commit. If the rewrite takes longer, I think you might want to merge this MR anyway, as it is low risk, fixes a bug, and provides a solution to an issue that users would otherwise have to wait for quite long.

Copy link
Contributor

@gaul gaul left a comment

Choose a reason for hiding this comment

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

I tested this PR with s3fs and gcsfuse and it makes testing with pjdfstest substantially less noisy. I prefer to merge this PR since I have some other workarounds that build on top of it.

@twied
Copy link
Contributor Author

twied commented Feb 20, 2022

@ngie-eign how is the Python rewrite coming along? And did you have a chance yet to consider the arguments I made in the last comments? I have rebased the patches onto current master, in case you change your mind. Let me know if there's anything else I can do to convince you that this PR is worthwhile.

@gaul
Copy link
Contributor

gaul commented Aug 27, 2023

Can we merge this PR? I am interested in s3fs using pjdfstest again and having better support for requirements will avoid a lot of hard-coding demonstrated in s3fs-fuse/s3fs-fuse#1882.

twied added 23 commits August 27, 2023 10:42
Signed-off-by: Tim Wiederhake <[email protected]>
This makes the output less confusing if "${r}" is empty.

Signed-off-by: Tim Wiederhake <[email protected]>
This helps diagnose which test is actually failing, as the test
number alone does little to help find a test in a test file.

Signed-off-by: Tim Wiederhake <[email protected]>
Signed-off-by: Tim Wiederhake <[email protected]>
This avoids overwriting variables of the same name. This happens e.g.
for the variable "type" in "create_file()" and is a prerequisite to
fix a bug in tests/rename/09.t in a later patch.

Signed-off-by: Tim Wiederhake <[email protected]>
This allows a later patch to remove some calls to chown.

Signed-off-by: Tim Wiederhake <[email protected]>
This allows a later patch to better group sections that require
root permissions.

Signed-off-by: Tim Wiederhake <[email protected]>
This allows a later patch to skip sleeping in disabled tests.

Signed-off-by: Tim Wiederhake <[email protected]>
This allows a later patch to skip querying file statistics for
disabled tests.

Signed-off-by: Tim Wiederhake <[email protected]>
One special case less.

Signed-off-by: Tim Wiederhake <[email protected]>
Sort by name and settle on unquoted spelling.

Signed-off-by: Tim Wiederhake <[email protected]>
Idea is to be able to turn on and off requirements for certain sections
of tests. If "skipmsg" is empty, no requirement is currently blocking
the execution of tests. If "skipmsg" is not empty, it contains the name
of the first unmet requirement.

Signed-off-by: Tim Wiederhake <[email protected]>
Creation of block and character devices typically require root permissions.

This is handy to mask out other unsupported file types, e.g. for testing a
custom / special-purpose file system implementation.

Signed-off-by: Tim Wiederhake <[email protected]>
This logic is currently spread out over the test files and repeated
several times. A later patch will remove these instances and replace
them with calls to "require" or "push_requirement".

Signed-off-by: Tim Wiederhake <[email protected]>
Signed-off-by: Tim Wiederhake <[email protected]>
Signed-off-by: Tim Wiederhake <[email protected]>
Signed-off-by: Tim Wiederhake <[email protected]>
Signed-off-by: Tim Wiederhake <[email protected]>
Signed-off-by: Tim Wiederhake <[email protected]>
This has been replaced by the more fine-grained ftype_* features.

Signed-off-by: Tim Wiederhake <[email protected]>
@twied
Copy link
Contributor Author

twied commented Aug 27, 2023

What an unexpected call from the past 😀

As a favor to @gaul I rebased the merge request and resolved the conflicts.
Pretty much straight forward, the only notable change is that I dropped commit 9697374 ("Use different names for variables in outer and inner loop"), as upstream has the very similar 9a90c29 ("Fix variable name collision in the rename tests") in place.

@gaul
Copy link
Contributor

gaul commented Aug 27, 2023

To make a point about how painful this is, I configure s3fs to run pjdfstest this way:

    prove -rv \
        ../../pjdfstest/tests/chflags/*.t \
        ../../pjdfstest/tests/chmod/0[4689].t \
        ../../pjdfstest/tests/chmod/10.t \
        ../../pjdfstest/tests/chown/0[4689].t \
        ../../pjdfstest/tests/chown/10.t \
        ../../pjdfstest/tests/ftruncate/0[147-9].t \
        ../../pjdfstest/tests/ftruncate/1[0134].t \
        ../../pjdfstest/tests/granular/*.t \
        ../../pjdfstest/tests/link/*.t \
        ../../pjdfstest/tests/mkdir/0[347-9].t \
        ../../pjdfstest/tests/mkdir/1[12]*.t \
        ../../pjdfstest/tests/mkfifo/0[3478].t \
        ../../pjdfstest/tests/mkfifo/1*.t \
        ../../pjdfstest/tests/mknod/0[479].t \
        ../../pjdfstest/tests/mknod/10.t \
        ../../pjdfstest/tests/open/0[49].t \
        ../../pjdfstest/tests/open/1*.t \
        ../../pjdfstest/tests/open/2[0-134].t \
        ../../pjdfstest/tests/posix_fallocate/*.t \
        ../../pjdfstest/tests/rename/0[2-36-8].t \
        ../../pjdfstest/tests/rename/1[15-9].t \
        ../../pjdfstest/tests/rename/22.t \
        ../../pjdfstest/tests/rmdir/0[13-59].t \
        ../../pjdfstest/tests/rmdir/1[02-5].t \
        ../../pjdfstest/tests/symlink/0[13479].t \
        ../../pjdfstest/tests/symlink/1*.t \
        ../../pjdfstest/tests/truncate/0[147-9].t \
        ../../pjdfstest/tests/truncate/1[0134].t \
        ../../pjdfstest/tests/unlink/0[347-8].t \
        ../../pjdfstest/tests/unlink/1[02-4].t \
        ../../pjdfstest/tests/utimensat/0[1-58-9].t

@gaul
Copy link
Contributor

gaul commented Jul 5, 2024

I guess the rust rewrite never panned out. Does anything block merging this PR?

@twied
Copy link
Contributor Author

twied commented Oct 13, 2024

Hi @ngie-eign! Just a friendly ping to ask about the status of this pull request. Is there anything I can do to make this pull request go forward?

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.

Excluding file types
3 participants