From: Anand Jain <anand.jain@oracle.com>
To: Filipe Manana <fdmanana@kernel.org>
Cc: fstests@vger.kernel.org, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v3 03/10] btrfs: create a helper function, check_fsid(), to verify the tempfsid
Date: Thu, 29 Feb 2024 07:20:47 +0530 [thread overview]
Message-ID: <6c50f2c1-407c-4ce8-a2e7-50cdd3b164a9@oracle.com> (raw)
In-Reply-To: <CAL3q7H56eR0BW_BW08i6+sfQ3VHyh2S7DuffNRob6GVEWDSFQA@mail.gmail.com>
On 2/28/24 15:58, Filipe Manana wrote:
> On Wed, Feb 28, 2024 at 9:36 AM Anand Jain <anand.jain@oracle.com> 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
next prev parent reply other threads:[~2024-02-29 1:50 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-24 16:43 [PATCH v3 00/10] btrfs: functional test cases for tempfsid Anand Jain
2024-02-24 16:43 ` [PATCH v3 01/10] assign SCRATCH_DEV_POOL to an array Anand Jain
2024-02-24 16:43 ` [PATCH v3 02/10] btrfs: introduce tempfsid test group Anand Jain
2024-02-24 16:43 ` [PATCH v3 03/10] btrfs: create a helper function, check_fsid(), to verify the tempfsid Anand Jain
2024-02-26 11:47 ` Filipe Manana
2024-02-28 9:36 ` Anand Jain
2024-02-28 10:28 ` Filipe Manana
2024-02-29 1:50 ` Anand Jain [this message]
2024-02-24 16:43 ` [PATCH v3 04/10] btrfs: verify that subvolume mounts are unaffected by tempfsid Anand Jain
2024-02-26 11:49 ` Filipe Manana
2024-02-24 16:43 ` [PATCH v3 05/10] btrfs: check if cloned device mounts with tempfsid Anand Jain
2024-02-26 11:55 ` Filipe Manana
2024-02-29 1:49 ` Anand Jain
2024-02-24 16:43 ` [PATCH v3 06/10] btrfs: test case prerequisite _require_btrfs_mkfs_uuid_option Anand Jain
2024-02-24 16:43 ` [PATCH v3 07/10] btrfs: introduce helper for creating cloned devices with mkfs Anand Jain
2024-02-26 11:57 ` Filipe Manana
2024-02-24 16:43 ` [PATCH v3 08/10] btrfs: verify tempfsid clones using mkfs Anand Jain
2024-02-26 11:59 ` Filipe Manana
2024-02-24 16:43 ` [PATCH v3 09/10] btrfs: validate send-receive operation with tempfsid Anand Jain
2024-02-26 12:06 ` Filipe Manana
2024-02-29 1:49 ` Anand Jain
2024-02-24 16:43 ` [PATCH v3 10/10] btrfs: test tempfsid with device add, seed, and balance Anand Jain
2024-02-26 12:08 ` Filipe Manana
2024-02-29 1:49 ` Anand Jain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6c50f2c1-407c-4ce8-a2e7-50cdd3b164a9@oracle.com \
--to=anand.jain@oracle.com \
--cc=fdmanana@kernel.org \
--cc=fstests@vger.kernel.org \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).