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. Thanks. > > Thanks, Anand
