Changeset View
Standalone View
tests/sys/cddl/zfs/tests/cli_root/zpool_add/zpool_add_005_pos.ksh
| Show First 20 Lines • Show All 64 Lines • ▼ Show 20 Lines | |||||
| function cleanup | function cleanup | ||||
| { | { | ||||
| poolexists "$TESTPOOL" && \ | poolexists "$TESTPOOL" && \ | ||||
| destroy_pool "$TESTPOOL" | destroy_pool "$TESTPOOL" | ||||
| poolexists "$TESTPOOL1" && \ | poolexists "$TESTPOOL1" && \ | ||||
| destroy_pool "$TESTPOOL1" | destroy_pool "$TESTPOOL1" | ||||
| $DUMPON -r $dump_dev | log_onfail $UMOUNT $TMPDIR/mounted_dir | ||||
| log_onfail $SWAPOFF $swap_dev | |||||
asomers: I disagree with this change. If anybody ever fixes bug 241070, it's unlikely that they'll… | |||||
Done Inline Actions
I just mentioned these tests in a comment. That will greatly increase the odds that they'll get fixed. But even then, we're only human, so I agree, they could get missed.
I don't want to make even more copies of these tests. I already had to fix the same issue in three nearly identical copies. I don't want to make it worse. I also don't have the time to refactor. How about this? I could remove the if $bug_241070_is_fixed conditions and invert the sense of the test (log_must zpool add ... dump_dev) with a comment referring to the bug. It's not an explicit expected failure, but it's practically the same and avoids all of the above issues. I'll code that up for review. vangyzen: > If anybody ever fixes bug 241070, it's unlikely that they'll think to update this test,
I… | |||||
| log_onfail $DUMPON -r $dump_dev | |||||
| } | } | ||||
Done Inline ActionsI know that many of the other ZFS tests do this, but it's really best not to use "log_must" in a cleanup function. Because if it fails, then it will prevent the rest of the cleanup function from running. asomers: I know that many of the other ZFS tests do this, but it's really best not to use "log_must" in… | |||||
Done Inline ActionsThanks. I admit, I was mostly on auto-pilot for the cleanup. vangyzen: Thanks. I admit, I was mostly on auto-pilot for the cleanup. | |||||
| log_assert "'zpool add' should fail with inapplicable scenarios." | log_assert "'zpool add' should fail with inapplicable scenarios." | ||||
| log_onexit cleanup | log_onexit cleanup | ||||
| mnttab_dev=$(find_mnttab_dev) | |||||
| vfstab_dev=$(find_vfstab_dev) | |||||
| dump_dev=${DISK2} | |||||
| create_pool "$TESTPOOL" "${DISK0}" | create_pool "$TESTPOOL" "${DISK0}" | ||||
| log_must poolexists "$TESTPOOL" | log_must poolexists "$TESTPOOL" | ||||
| create_pool "$TESTPOOL1" "${DISK1}" | create_pool "$TESTPOOL1" "${DISK1}" | ||||
| log_must poolexists "$TESTPOOL1" | log_must poolexists "$TESTPOOL1" | ||||
| log_mustnot $ZPOOL add -f "$TESTPOOL" ${DISK1} | |||||
| log_mustnot $ZPOOL add -f "$TESTPOOL" $mnttab_dev | mounted_dev=${DISK2} | ||||
| log_must $MKDIR $TMPDIR/mounted_dir | |||||
| log_must $NEWFS $mounted_dev | |||||
| log_must $MOUNT $mounted_dev $TMPDIR/mounted_dir | |||||
| log_mustnot $ZPOOL add -f "$TESTPOOL" $vfstab_dev | swap_dev=${DISK3} | ||||
| log_must $SWAPON $swap_dev | |||||
| dump_dev=${DISK4} | |||||
| log_must $DUMPON $dump_dev | log_must $DUMPON $dump_dev | ||||
| log_mustnot $ZPOOL add -f "$TESTPOOL" $dump_dev | |||||
| log_mustnot $ZPOOL add -f "$TESTPOOL" ${DISK1} | |||||
| log_mustnot $ZPOOL add -f "$TESTPOOL" $mounted_dev | |||||
| log_mustnot $ZPOOL add -f "$TESTPOOL" $swap_dev | |||||
| # https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=241070 | |||||
| # When that bug is fixed, change this to log_mustnot. | |||||
| log_must $ZPOOL add -f "$TESTPOOL" $dump_dev | |||||
| log_pass "'zpool add' should fail with inapplicable scenarios." | log_pass "'zpool add' should fail with inapplicable scenarios." | ||||
I disagree with this change. If anybody ever fixes bug 241070, it's unlikely that they'll think to update this test, and we'll continue to lack coverage.
Having an expected_failure test fail for the "wrong" reason is a real hazard, but it's less bad than the alternatives, such as simply disabling the test.
In this case, might I suggest moving the dump_dev assertion into a separate standalone test case so it can still be marked with atf_expect_fail, while all of the other stuff can remain here and hopefully pass?