mbox series

[blktests,0/3] Add --config argument for custom config filenames

Message ID 20191029200942.83044-1-andrealmeid@collabora.com (mailing list archive)
Headers show
Series Add --config argument for custom config filenames | expand

Message

André Almeida Oct. 29, 2019, 8:09 p.m. UTC
Instead of just using the default config file, one may also find useful to
specify which configuration file would like to use without editing the config
file, like this:

$ ./check --config=tests_nvme
...
$ ./check -c tests_scsi

This pull request solves this. This change means to be optional, in the sense
that the default behavior should not be modified and current setups will not be
affect by this. To check if this is true, I have done the following test:

- Print the value of variables $DEVICE_ONLY, $QUICK_RUN, $TIMEOUT,
  $RUN_ZONED_TESTS, $OUTPUT, $EXCLUDE
  
- Run with the following setups:
    - with a config file in the dir
    - without a config file in the dir
    - configuring using command line arguments

With both original code and with my changes, I validated that the values
remained the same. Then, I used the argument --config=test_config to check that
the values of variables are indeed changing.

This patchset add this feature, update the docs and fix a minor issue with a
command line argument. Also, I have changed "# shellcheck disable=SC1091" to
"# shellcheck source=/dev/null", since it seems the proper way to disable this
check according to shellcheck documentation[1].

Thanks,
André

[1] https://github.com/koalaman/shellcheck/wiki/SC1090#exceptions

This patch is also avaible at GitHub:
https://github.com/osandov/blktests/pull/56

André Almeida (3):
  check: Add configuration file option
  Documentation: Add information about `--config` argument
  check: Make "device-only" option a valid option

 Documentation/running-tests.md |  3 ++-
 check                          | 29 ++++++++++++++++++++++++-----
 2 files changed, 26 insertions(+), 6 deletions(-)

Comments

Omar Sandoval Oct. 30, 2019, 9:18 p.m. UTC | #1
On Tue, Oct 29, 2019 at 05:09:39PM -0300, André Almeida wrote:
> Instead of just using the default config file, one may also find useful to
> specify which configuration file would like to use without editing the config
> file, like this:
> 
> $ ./check --config=tests_nvme
> ...
> $ ./check -c tests_scsi
> 
> This pull request solves this. This change means to be optional, in the sense
> that the default behavior should not be modified and current setups will not be
> affect by this. To check if this is true, I have done the following test:
> 
> - Print the value of variables $DEVICE_ONLY, $QUICK_RUN, $TIMEOUT,
>   $RUN_ZONED_TESTS, $OUTPUT, $EXCLUDE
>   
> - Run with the following setups:
>     - with a config file in the dir
>     - without a config file in the dir
>     - configuring using command line arguments
> 
> With both original code and with my changes, I validated that the values
> remained the same. Then, I used the argument --config=test_config to check that
> the values of variables are indeed changing.
> 
> This patchset add this feature, update the docs and fix a minor issue with a
> command line argument. Also, I have changed "# shellcheck disable=SC1091" to
> "# shellcheck source=/dev/null", since it seems the proper way to disable this
> check according to shellcheck documentation[1].
> 
> Thanks,
> André
> 
> [1] https://github.com/koalaman/shellcheck/wiki/SC1090#exceptions
> 
> This patch is also avaible at GitHub:
> https://github.com/osandov/blktests/pull/56
> 
> André Almeida (3):
>   check: Add configuration file option
>   Documentation: Add information about `--config` argument
>   check: Make "device-only" option a valid option

Patches 2 and 3 look good (although a nitpick is that patch 3 could be
first since it's a bug fix that I could take independently of the other
patches). I had one comment on patch 1.

Thanks!
André Almeida Oct. 30, 2019, 10:14 p.m. UTC | #2
On 10/30/19 6:18 PM, Omar Sandoval wrote:
> On Tue, Oct 29, 2019 at 05:09:39PM -0300, André Almeida wrote:
>> Instead of just using the default config file, one may also find useful to
>> specify which configuration file would like to use without editing the config
>> file, like this:
>>
>> $ ./check --config=tests_nvme
>> ...
>> $ ./check -c tests_scsi
>>
>> This pull request solves this. This change means to be optional, in the sense
>> that the default behavior should not be modified and current setups will not be
>> affect by this. To check if this is true, I have done the following test:
>>
>> - Print the value of variables $DEVICE_ONLY, $QUICK_RUN, $TIMEOUT,
>>   $RUN_ZONED_TESTS, $OUTPUT, $EXCLUDE
>>   
>> - Run with the following setups:
>>     - with a config file in the dir
>>     - without a config file in the dir
>>     - configuring using command line arguments
>>
>> With both original code and with my changes, I validated that the values
>> remained the same. Then, I used the argument --config=test_config to check that
>> the values of variables are indeed changing.
>>
>> This patchset add this feature, update the docs and fix a minor issue with a
>> command line argument. Also, I have changed "# shellcheck disable=SC1091" to
>> "# shellcheck source=/dev/null", since it seems the proper way to disable this
>> check according to shellcheck documentation[1].
>>
>> Thanks,
>> André
>>
>> [1] https://github.com/koalaman/shellcheck/wiki/SC1090#exceptions
>>
>> This patch is also avaible at GitHub:
>> https://github.com/osandov/blktests/pull/56
>>
>> André Almeida (3):
>>   check: Add configuration file option
>>   Documentation: Add information about `--config` argument
>>   check: Make "device-only" option a valid option
> 
> Patches 2 and 3 look good (although a nitpick is that patch 3 could be
> first since it's a bug fix that I could take independently of the other
> patches). I had one comment on patch 1.
> 

Nice catch, the order will be fixed at v2 :)

> Thanks!
>