On 2/28/24 15:58, Filipe Manana wrote:
On Wed, Feb 28, 2024 at 9:36 AM Anand Jain <anand.jain@xxxxxxxxxx> wrote:
function should do the require for everything it needs that may not be
available.
It's doing for the inspect-internal command, but it's missing a:
_require_btrfs_sysfs_fsid
Yes, it did. Actually, check_fsid() would need the following to
cover all the prerequisites.
_require_btrfs_fs_sysfs
_require_btrfs_fs_feature temp_fsid
_require_btrfs_fs_feature metadata_uuid
_require_btrfs_command inspect-internal dump-super
I already have v4 with what you just suggested, I am going to send it.
> Instead this is being called for every test case that calls this new
> helper function, when those requirements should be hidden from the
> tests themselves.
However, I am a bit skeptical if we should move all prerequisites to
the helpers or only some major prerequisites.
Because returning _notrun() in the middle of the testcase is something
I am not sure is better than at the beginning of the testcase (I do not
have a specific example where it is not a good idea, though).
And, theoretically, figuring out if the test case would run/_notrun()
will be complicated.
Next, we shall end up checking the _require..() multiple times in
a test case, though one time is enough (the test cases 311, 312,
313 call check_fsid() two times).
Furthermore, it will inconsistent, as a lot of command wraps are
already missing such a requirement; I'm not sure if we shall ever
achieve consistency across fstests (For example: _cp_reflink()
missing _require_cp_reflink).
Lastly, if there are duplicating prerequisites across the helper
functions, then we call _require..() many more times (for example:
313 will call mkfs_clone() and check_fsid() two times, which
means we would verify the following three times in a testcase.
_require_btrfs_fs_feature metadata_uuid
_require_btrfs_command inspect-internal dump-super
So, how about prerequisites of the newer functions as comments
above the function to be copied into the test case?
Calling the require functions doesn't take that much time, I'm not
worried about more 1, 2, 3 or 10 milliseconds of test run time.
Now having each test that uses a common function to call all the
require functions is hard to maintain and messy.
Commenting the requirements on top of each function is not bullet
proof - test authors will have to do it and reviewers as well all the
time.
Not to mention that if a function's implementation changes and now it
has different requirements, we'll have to change every single test
that uses it.
I was trying to keep the code optimized and avoid duplicate '_require..'
statements as much as possible. Also, I aimed to avoid '_notrun' in the
middle of the testcase, which keeps it inline with the rest of the older
testcases. However, it seems not to be a big deal, so let the
'_require..' statements be in the helpers. This makes the test case
look more concise and further makes it easy to make changes. For
example, if a helper is deleted, the testcase will still be fine
without bugs. I have updated the rest of the test cases with this
idea in v4.
Thanks, Anand
