在 2024/4/10 13:46, Anand Jain 写道:
On 4/9/24 19:13, David Sterba wrote:
On Mon, Apr 08, 2024 at 12:46:18PM +0800, Anand Jain wrote:
On 4/6/24 13:18, Qu Wenruo wrote:
[BUG]
All the touched test cases would fail after btrfs-progs commit
5f87b467a9e7 ("btrfs-progs: subvolume: output the prompt line only when
the ioctl succeeded") due to golden output mismatch.
[CAUSE]
Although the patch I sent to the mail list doesn't change the output at
all but only a timing change, David uses this patch to unify the output
of "btrfs subvolume create" and "btrfs subvolume snapshot".
Unfortunately this changes the output and causes mismatch with
golden output.
[FIX]
Just redirect stdout of "btrfs subvolume snapshot" to $seqres.full.
Any error from "btrfs subvolume" subgroup would lead to error messages
into stderr, and cause golden output mismatch.
This can be comprehensively greped by
'grep -IR "Create a" tests/btrfs/*.out' command.
In fact, we have around 274 "btrfs subvolume snapshot|create" calls
in the
existing test cases, meanwhile only around 61 calls are populating
golden output (22 for subvolume creation, and 39 for snapshot
creation).
Thus majority of the snapshot/subvolume creation is not populating
golden output already.
While golden output is better verification method in terms of
accuracy, but, it falls short in verifying command exit codes.
I personally think the run_btrfs_progs approach is better for
'btrfs subvolume snapshot'. It allows us to verify the command
status without relying on the stdout.
But, past discussions favored the golden output verification
method instead of run_btrfs_progs.
I thought the whole point here was to depart from the golden output, at
least in selected cases, and only in btrfs/ subdirectory so it does not
accidentally break other filesystems' testing.
What past discussions favored does not seem to satisfy our needs and as
btrfs-progs are evolving we're hitting random test breakage just because
some message has changed. The testsuite should verify what matters, ie.
return code, state of the filesystem etc, not exact command output.
There's high correlation between output and correctness, yes, but this
is too fragile.
Agreed. So, why don't we use `_run_btrfs_util_prog subvolume
snapshot`, which makes it consistent with the rest of the test cases,
and also remove the golden output for this command?
For `_run_btrfs_util_prog`, the only thing I do not like is the name itself.
I also do not like how fstests always go $BTRFS_UTIL_PROG neither,
however I understand it's there to make sure we do not got weird bash
function name like "btrfs()" overriding the real "btrfs".
If we can make the name shorter like `_btrfs` or something like it, I'm
totally fine with that, and would be happy to move to the new interface.
In fact, `_run_btrfs_util_prog` is pretty helpful to generate a debug
friendly seqres.full, which is another good point.
Thanks,
Qu
Thanks, Anand
