Hi Shinichiro,
On Wed, Apr 03, 2024 at 04:54:28AM +0000, Shinichiro Kawasaki wrote:
> 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.
Yes, I agree. These drawbacks should be addressed somehow.
> 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?
Ideally we don't have to introduce additional common setup logic into
each test. Also for debugging purpose it might sense to run a test only
in one configuration. So it might make sense still to have user visible
environment variable
nvmet_blkdev_types="file device"
as default.
> -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
I was wondering if the nvmet_run_for_each_blkdev_type logic could be in
common code, so we don't have to add a do_test function. Basically
having a common code for a bunch of configuration variables (matrix
tests). This could also be useful for nvmet_trtype etc.
The generic setup could be requested via the require hook.
requires() = {
_nvmet_setup_target
}
What do you think about this idea?
Thanks,
Daniel
