On Apr 02, 2024 / 12:03, Daniel Wagner wrote:
> During the last batch of refactoring I noticed that we could reduce the number
> of tests a bit. There are tests which are almost identically except how the
> target is configured, file vs block device backend.
>
> By introducing a configure knob, we can drop the duplicates and even make some
> of the tests a bit more versatile. Not all tests exists in file and block device
> backend version. Thus we increase the test coverage with this series. And not
> really surprising, there is a fallout. The nvme/028 test with file backend is
> failing in my setup but I was not able to figure out where things go wrong yet.
> I'll provide some logs later.
Hi Daniel, I find this series reduces code duplications further, and it expands
test coverage with small test script changes. Good.
On the other hand, I see that the series has a couple of drawbacks:
1) When blktests users run with the default knob only, the test coverage will be
smaller. To keep the current test coverage, the users need to run the check
script twice: nvmet_blkdev_type=file and nvmet_blkdev_type=device. Some users
may not do it and lose the test coverage. And some users, e.g., CKI project,
need to adjust their script for this change.
2) When the users run the check script twice to keep the test coverage, some
test cases are executed twice under the exact same test conditions. This
will waste some of the test run effort.
To avoid the drawbacks, how about this?
- Do not provide nvmet_blkdev_type as a knob for users. Keep it as just a global
variable in tests/nvme/rc. (It should be renamed to clarify that it is not for
users.)
- Introduce a helper function to do the same test twice for nvmet_blkdev_type=
file and nvmet_blkdev_type=device. Call this helper function from a single
test case to cover both the blkdev types.
I attach a patch at the end of this email to show the ideas above. It applies
the idea to nvme/006 as an example. What do you think?
diff --git a/tests/nvme/006 b/tests/nvme/006
index 65f2a0d..6241961 100755
--- a/tests/nvme/006
+++ b/tests/nvme/006
@@ -15,14 +15,18 @@ requires() {
_require_nvme_trtype_is_fabrics
}
-test() {
- echo "Running ${TEST_NAME}"
-
+do_test() {
_setup_nvmet
_nvmet_target_setup
_nvmet_target_cleanup
+}
+
+test() {
+ echo "Running ${TEST_NAME}"
+
+ _nvmet_run_for_each_blkdev_type do_test
echo "Test complete"
}
diff --git a/tests/nvme/rc b/tests/nvme/rc
index 5fa1871..4155411 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -996,6 +996,15 @@ _nvmet_passthru_target_cleanup() {
_remove_nvmet_host "${def_hostnqn}"
}
+_nvmet_run_for_each_blkdev_type() {
+ local blkdev_type
+
+ for blkdev_type in device file; do
+ nvmet_blkdev_type=$blkdev_type
+ eval "$1"
+ done
+}
+
_discovery_genctr() {
_nvme_discover "${nvme_trtype}" |
sed -n -e 's/^.*Generation counter \([0-9]\+\).*$/\1/p'
