Re: [PATCH 02/21] tests: Add bash lib

Re: [PATCH 02/21] tests: Add bash lib

[Jonatan Schlag]

[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]


Hi,


Am Freitag, dem 31.05.2024 um 10:47 +0100 schrieb Michael Tremer:
> Hello,
> 
> > On 20 May 2024, at 10:05, Jonatan Schlag
> > <jonatan.schlag(a)ipfire.org> wrote:
> > 
> > This allows use to write test with less effort as we can reuse
> > functions
> > 
> > Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> > ---
> > tests/lib.sh | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> > create mode 100644 tests/lib.sh
> > 
> > diff --git a/tests/lib.sh b/tests/lib.sh
> > new file mode 100644
> > index 000000000..7749d5158
> > --- /dev/null
> > +++ b/tests/lib.sh
> > @@ -0,0 +1,20 @@
> > +#!/usr/bin/bash
> > +
> > +LIB_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")"
> 
> ???
> 
> I don’t think this is very intuitive.

Ok, what am I supposed to do about it? I need the path where this file
is located, and I cannot hard-code it. Should I add a comment?

Jonatan

> 
> > +. ${LIB_DIR}/lib_color.sh
> > +
> > +test_that() {
> > +
> > + if ! "$@" ; then
> > + echo -e "${CLR_RED_BG} Test failed: ${*} ${CLR_RESET}"
> > + return 1
> > + else
> > + echo -e "${CLR_GREEN_BG} Test succeded: ${*} ${CLR_RESET}"
> > + return 0
> > + fi
> > +}
> 
> We have had so many discussions about how to name functions. “That”
> is quite generic in my opinion.
> 
> Why not “test_command” or something like that?
> 
> > +
> > +var_has_value() {
> > + [[ "${!1}" == "${2}" ]]
> > +}
> > -- 
> > 2.39.2
> > 
> 

Re: [PATCH 02/21] tests: Add bash lib

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 2237 bytes --]

Hello,

> On 2 Jun 2024, at 18:49, Jonatan Schlag <jonatan.schlag(a)ipfire.org> wrote:
> 
> 
> Hi,
> 
> 
> Am Freitag, dem 31.05.2024 um 10:47 +0100 schrieb Michael Tremer:
>> Hello,
>> 
>>> On 20 May 2024, at 10:05, Jonatan Schlag
>>> <jonatan.schlag(a)ipfire.org> wrote:
>>> 
>>> This allows use to write test with less effort as we can reuse
>>> functions
>>> 
>>> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
>>> ---
>>> tests/lib.sh | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>> create mode 100644 tests/lib.sh
>>> 
>>> diff --git a/tests/lib.sh b/tests/lib.sh
>>> new file mode 100644
>>> index 000000000..7749d5158
>>> --- /dev/null
>>> +++ b/tests/lib.sh
>>> @@ -0,0 +1,20 @@
>>> +#!/usr/bin/bash
>>> +
>>> +LIB_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")"
>> 
>> ???
>> 
>> I don’t think this is very intuitive.
> 
> Ok, what am I supposed to do about it? I need the path where this file
> is located, and I cannot hard-code it. Should I add a comment?

Generally speaking, I like to split things into one line per task. So that it is very easy to see what is happening when reading the code. This approach also has some more advantages. For example, you can easily put a print statement between lines to see if you are getting the output that you are expecting. That makes debugging nice and easy.

When I say generally, there are of course exceptions like very trivial things that can often be put into the same line.

Commenting is also a lot easier when you only have to comment one thing at once. You need to add way more comments, please.

-Michael

> Jonatan
> 
>> 
>>> +. ${LIB_DIR}/lib_color.sh
>>> +
>>> +test_that() {
>>> +
>>> + if ! "$@" ; then
>>> + echo -e "${CLR_RED_BG} Test failed: ${*} ${CLR_RESET}"
>>> + return 1
>>> + else
>>> + echo -e "${CLR_GREEN_BG} Test succeded: ${*} ${CLR_RESET}"
>>> + return 0
>>> + fi
>>> +}
>> 
>> We have had so many discussions about how to name functions. “That”
>> is quite generic in my opinion.
>> 
>> Why not “test_command” or something like that?
>> 
>>> +
>>> +var_has_value() {
>>> + [[ "${!1}" == "${2}" ]]
>>> +}
>>> -- 
>>> 2.39.2

First patch series for the roadmap item "Get rid of CONFIG_TYPE in the network config"

[Jonatan Schlag]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

Hi list,

This is the first patch series for the roadmap item: https://www.ipfire.org/docs/roadmap/get-rid-of-configtype-in-network-config . This patch series introduces a bash testing library and a function readhash, which we will use in every bash script which needs to read our config files. As this patch set is already massive, I thought it might be better to discuss the function first before I introduce it in every script. Otherwise, we would talk about 50 patches and not 21. This amount would be somehow not reviewable. If this patch series get merged, fine. If you want to wait for all the patches also fine, but as told, this will get a huge patch set.

Patches in this series are:

[PATCH 01/21] test: Add bash lib for colors
[PATCH 02/21] tests: Add bash lib
[PATCH 03/21] tests/lib.sh: Add function
[PATCH 04/21] tests/lib.sh: Add check if variable exists to
[PATCH 05/21] tests/lib.sh: Add logging functions
[PATCH 06/21] tests/lib.sh: adjust to pytest logging style
[PATCH 07/21] tests/lib.sh: Fix check if array is defined
[PATCH 08/21] test_that_key_in_arry_has_value: Check if the key is
[PATCH 09/21] tests/lib.sh: Use namref to access an array
[PATCH 10/21] test_that_key_in_arry_has_value: Check if a key in an
[PATCH 11/21] tests: Add function to test the ouput of a bash
[PATCH 12/21] initscript functions: add readhash
[PATCH 13/21] initscript fkt: ignore blank lines in readhash
[PATCH 14/21] test: Add functions test_that_array_is_defined
[PATCH 15/21] tests: Add functions test_that_array_doesnt_have_key
[PATCH 16/21] initscripts fkt: Ignore comments in readhash
[PATCH 17/21] initscripts fkt: ignore invalid keys in readhash
[PATCH 18/21] initscripts fkt: readhash should only parse lines with
[PATCH 19/21] initscripts fkt: Check for invalid values in readhash
[PATCH 20/21] initscripts fkt: keep readhash compatible with older
[PATCH 21/21] initscripts fkt: Fix shebang

Greetings Jonatan

[PATCH 02/21] tests: Add bash lib

[Jonatan Schlag]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

This allows use to write test with less effort as we can reuse functions

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 tests/lib.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 tests/lib.sh

diff --git a/tests/lib.sh b/tests/lib.sh
new file mode 100644
index 000000000..7749d5158
--- /dev/null
+++ b/tests/lib.sh
@@ -0,0 +1,20 @@
+#!/usr/bin/bash
+
+LIB_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")"
+
+. ${LIB_DIR}/lib_color.sh
+
+test_that() {
+
+	if ! "$@" ; then
+		echo -e "${CLR_RED_BG} Test failed: ${*} ${CLR_RESET}"
+		return 1
+	else
+		echo -e "${CLR_GREEN_BG} Test succeded: ${*} ${CLR_RESET}"
+		return 0
+	fi
+}
+
+var_has_value() {
+	[[ "${!1}" == "${2}" ]]
+}
-- 
2.39.2