From: Geliang Tang <tanggeliang@kylinos.cn>
This patch adds two vars in mptcp_lib.sh, MPTCP_LIB_TEST_COUNTER for
the test counter and MPTCP_LIB_TEST_FORMAT for the test print format.
Also add two helpers, mptcp_lib_inc_test_counter(), increase the test
counter, and mptcp_lib_pr_title_counter(), print the test title with
counter. They are used in mptcp_join.sh first.
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
tools/testing/selftests/net/mptcp/mptcp_join.sh | 16 ++++++++--------
tools/testing/selftests/net/mptcp/mptcp_lib.sh | 17 +++++++++++++++++
2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 1df2d24979a0..7acc6064eb17 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -48,7 +48,6 @@ declare -A all_tests
declare -a only_tests_ids
declare -a only_tests_names
declare -A failed_tests
-TEST_COUNT=0
TEST_NAME=""
nr_blank=6
@@ -172,7 +171,8 @@ cleanup()
print_title()
{
- printf "%03u %s\n" "${TEST_COUNT}" "${TEST_NAME}"
+ MPTCP_LIB_TEST_FORMAT="%03u %s\n"
+ mptcp_lib_pr_title_counter "${TEST_NAME}"
}
print_check()
@@ -233,7 +233,7 @@ skip_test()
local i
for i in "${only_tests_ids[@]}"; do
- if [ "${TEST_COUNT}" -eq "${i}" ]; then
+ if [ "${MPTCP_LIB_TEST_COUNTER}" -eq "${i}" ]; then
return 1
fi
done
@@ -268,7 +268,7 @@ reset()
TEST_NAME="${1}"
- TEST_COUNT=$((TEST_COUNT+1))
+ mptcp_lib_inc_test_counter
if skip_test; then
last_test_ignored=1
@@ -462,7 +462,7 @@ fail_test()
# just in case a test is marked twice as failed
if [ ${last_test_failed} -eq 0 ]; then
- failed_tests[${TEST_COUNT}]="${TEST_NAME}"
+ failed_tests[${MPTCP_LIB_TEST_COUNTER}]="${TEST_NAME}"
dump_stats
last_test_failed=1
fi
@@ -973,7 +973,7 @@ do_transfer()
local srv_proto="$4"
local connect_addr="$5"
- local port=$((10000 + TEST_COUNT - 1))
+ local port=$((10000 + MPTCP_LIB_TEST_COUNTER - 1))
local cappid
local FAILING_LINKS=${FAILING_LINKS:-""}
local fastclose=${fastclose:-""}
@@ -991,9 +991,9 @@ do_transfer()
capuser="-Z $SUDO_USER"
fi
- capfile=$(printf "mp_join-%02u-%s.pcap" "$TEST_COUNT" "${listener_ns}")
+ capfile=$(printf "mp_join-%02u-%s.pcap" "$MPTCP_LIB_TEST_COUNTER" "${listener_ns}")
- echo "Capturing traffic for test $TEST_COUNT into $capfile"
+ echo "Capturing traffic for test $MPTCP_LIB_TEST_COUNTER into $capfile"
ip netns exec ${listener_ns} tcpdump -i any -s 65535 -B 32768 $capuser -w $capfile > "$capout" 2>&1 &
cappid=$!
diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
index 763a2989ca6d..cbf0dd2cc4cb 100644
--- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
@@ -411,3 +411,20 @@ mptcp_lib_events() {
ip netns exec "${ns}" ./pm_nl_ctl events >> "${evts}" 2>&1 &
pid=$!
}
+
+MPTCP_LIB_TEST_COUNTER=0
+MPTCP_LIB_TEST_FORMAT=""
+
+mptcp_lib_inc_test_counter() {
+ : "${MPTCP_LIB_TEST_COUNTER:?}"
+
+ MPTCP_LIB_TEST_COUNTER=$((MPTCP_LIB_TEST_COUNTER+1))
+}
+
+#shellcheck disable=SC2059
+mptcp_lib_pr_title_counter() {
+ : "${MPTCP_LIB_TEST_COUNTER:?}"
+ : "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
+
+ printf "${MPTCP_LIB_TEST_FORMAT}" "${MPTCP_LIB_TEST_COUNTER}" "${*}"
+}
--
2.40.1Hi Geliang,
On 29/02/2024 10:51, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> This patch adds two vars in mptcp_lib.sh, MPTCP_LIB_TEST_COUNTER for
> the test counter and MPTCP_LIB_TEST_FORMAT for the test print format.
> Also add two helpers, mptcp_lib_inc_test_counter(), increase the test
> counter, and mptcp_lib_pr_title_counter(), print the test title with
> counter. They are used in mptcp_join.sh first.
Please add the reason, something like: Each MPTCP selftest is having
subtests, and it helps to give them a number to quickly identify them.
This can be managed by mptcp_lib.sh, reusing what has been done here.
The following commit will use these new helpers in the other tests.
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> tools/testing/selftests/net/mptcp/mptcp_join.sh | 16 ++++++++--------
> tools/testing/selftests/net/mptcp/mptcp_lib.sh | 17 +++++++++++++++++
> 2 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 1df2d24979a0..7acc6064eb17 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -48,7 +48,6 @@ declare -A all_tests
> declare -a only_tests_ids
> declare -a only_tests_names
> declare -A failed_tests
> -TEST_COUNT=0
> TEST_NAME=""
> nr_blank=6
>
> @@ -172,7 +171,8 @@ cleanup()
>
> print_title()
> {
> - printf "%03u %s\n" "${TEST_COUNT}" "${TEST_NAME}"
> + MPTCP_LIB_TEST_FORMAT="%03u %s\n"
No need to re-set it all the time, this line can be moved just before
"sourcing" mptcp_lib.sh I think, with a comment like:
# Here, we have more than 100 tests, having multiple checks
MPTCP_LIB_TEST_FORMAT="%03u %s\n"
. "$(dirname "${0}")/mptcp_lib.sh"
> + mptcp_lib_pr_title_counter "${TEST_NAME}"
> }
>
> print_check()
(...)
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_lib.sh b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> index 763a2989ca6d..cbf0dd2cc4cb 100644
> --- a/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_lib.sh
> @@ -411,3 +411,20 @@ mptcp_lib_events() {
> ip netns exec "${ns}" ./pm_nl_ctl events >> "${evts}" 2>&1 &
> pid=$!
> }
> +
> +MPTCP_LIB_TEST_COUNTER=0
> +MPTCP_LIB_TEST_FORMAT=""
Can you declare them at the top, around the other ones?
(MPTCP_LIB_SUBTESTS, etc.)
And I think you should set the default value there too, like what I was
suggesting in patch 07/12 from v5:
: "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
MPTCP_LIB_TEST_COUNTER=0
So we can change the format by setting MPTCP_LIB_TEST_FORMAT before
"sourcing" mptcp_lib.sh.
No?
*If* we need to alter the format just for one title, we can always do:
MPTCP_LIB_TEST_FORMAT="..." \
mptcp_lib_pr_title_counter "<title>"
But best to avoid that.
> +
> +mptcp_lib_inc_test_counter() {
> + : "${MPTCP_LIB_TEST_COUNTER:?}"
I don't think that's needed, it has been defined in the file, no?
> +
> + MPTCP_LIB_TEST_COUNTER=$((MPTCP_LIB_TEST_COUNTER+1))
> +}
> +
> +#shellcheck disable=SC2059
Please always justify why you need to ignore a rule.
Also, always try to reduce the scope as much as possible: just for one
line, for one function, for the whole file → here, move it just before
calling 'printf':
# shellcheck disable=SC2059 # the format is in a variable
printf "${MPTCP_LIB_TEST_FORMAT}" (...)
(and add a space after '#')
> +mptcp_lib_pr_title_counter() {
> + : "${MPTCP_LIB_TEST_COUNTER:?}"
> + : "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
> +
> + printf "${MPTCP_LIB_TEST_FORMAT}" "${MPTCP_LIB_TEST_COUNTER}" "${*}"
I didn't check: can you not print the title and increment the counter
(via mptcp_lib_inc_test_counter) from here? I think that would be a good
practice to do that instead of having to deal with the counter in
different places.
We would only call "mptcp_lib_inc_test_counter()" from other scripts
when a test is skipped.
> +}
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
On 29/02/2024 12:58, Matthieu Baerts wrote:
> Hi Geliang,
>
> On 29/02/2024 10:51, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> This patch adds two vars in mptcp_lib.sh, MPTCP_LIB_TEST_COUNTER for
>> the test counter and MPTCP_LIB_TEST_FORMAT for the test print format.
>> Also add two helpers, mptcp_lib_inc_test_counter(), increase the test
>> counter, and mptcp_lib_pr_title_counter(), print the test title with
>> counter. They are used in mptcp_join.sh first.
>
> Please add the reason, something like: Each MPTCP selftest is having
> subtests, and it helps to give them a number to quickly identify them.
> This can be managed by mptcp_lib.sh, reusing what has been done here.
> The following commit will use these new helpers in the other tests.
>
>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>> ---
(...)
>> +mptcp_lib_pr_title_counter() {
>> + : "${MPTCP_LIB_TEST_COUNTER:?}"
>> + : "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
>> +
>> + printf "${MPTCP_LIB_TEST_FORMAT}" "${MPTCP_LIB_TEST_COUNTER}" "${*}"
>
> I didn't check: can you not print the title and increment the counter
> (via mptcp_lib_inc_test_counter) from here? I think that would be a good
> practice to do that instead of having to deal with the counter in
> different places.
>
> We would only call "mptcp_lib_inc_test_counter()" from other scripts
> when a test is skipped.
OK, I just saw you added mptcp_lib_print_title() in the next patch. Why
did you not use it for mptcp_join.sh?
Can you not call "mptcp_lib_inc_test_counter" in the "if skip_test" from
mptcp_join.sh and use mptcp_lib_print_title()? (if you do that, I guess
we don't need mptcp_lib_pr_title_counter(), no?)
>
>> +}
>
> Cheers,
> Matt
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
On Thu, 2024-02-29 at 13:28 +0100, Matthieu Baerts wrote:
> On 29/02/2024 12:58, Matthieu Baerts wrote:
> > Hi Geliang,
> >
> > On 29/02/2024 10:51, Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > >
> > > This patch adds two vars in mptcp_lib.sh, MPTCP_LIB_TEST_COUNTER
> > > for
> > > the test counter and MPTCP_LIB_TEST_FORMAT for the test print
> > > format.
> > > Also add two helpers, mptcp_lib_inc_test_counter(), increase the
> > > test
> > > counter, and mptcp_lib_pr_title_counter(), print the test title
> > > with
> > > counter. They are used in mptcp_join.sh first.
> >
> > Please add the reason, something like: Each MPTCP selftest is
> > having
> > subtests, and it helps to give them a number to quickly identify
> > them.
> > This can be managed by mptcp_lib.sh, reusing what has been done
> > here.
> > The following commit will use these new helpers in the other tests.
> >
> > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > ---
>
> (...)
>
> > > +mptcp_lib_pr_title_counter() {
> > > + : "${MPTCP_LIB_TEST_COUNTER:?}"
> > > + : "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
> > > +
> > > + printf "${MPTCP_LIB_TEST_FORMAT}"
> > > "${MPTCP_LIB_TEST_COUNTER}" "${*}"
> >
> > I didn't check: can you not print the title and increment the
> > counter
> > (via mptcp_lib_inc_test_counter) from here? I think that would be a
> > good
> > practice to do that instead of having to deal with the counter in
> > different places.
> >
> > We would only call "mptcp_lib_inc_test_counter()" from other
> > scripts
> > when a test is skipped.
>
> OK, I just saw you added mptcp_lib_print_title() in the next patch.
> Why
> did you not use it for mptcp_join.sh?
>
> Can you not call "mptcp_lib_inc_test_counter" in the "if skip_test"
> from
> mptcp_join.sh and use mptcp_lib_print_title()? (if you do that, I
> guess
> we don't need mptcp_lib_pr_title_counter(), no?)
Yes, I dropped mptcp_lib_pr_title_counter in v9.
>
> >
> > > +}
> >
> > Cheers,
> > Matt
>
> Cheers,
> Matt
On 29/02/2024 12:58, Matthieu Baerts wrote:
(...)
> And I think you should set the default value there too, like what I was
> suggesting in patch 07/12 from v5:
>
> : "${MPTCP_LIB_TEST_FORMAT:="%02u %-50s"}"
> MPTCP_LIB_TEST_COUNTER=0
>
> So we can change the format by setting MPTCP_LIB_TEST_FORMAT before
> "sourcing" mptcp_lib.sh.
Mmh, we can also simply set it here:
MPTCP_LIB_TEST_FORMAT="%02u %-50s"
MPTCP_LIB_TEST_COUNTER=0
And users of the lib can override it after having "sourced"
mptcp_lib.sh, along with other "global" variables set at the beginning
of the file (with a comment justifying why we need to alter the format).
(...)
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
© 2016 - 2024 Red Hat, Inc.