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'