Fix tests

[heeplr]

This fixes tests that fail on:

• ./configure ... --without-libmodbus (tests/mb2hal) - disable tests when building w/o libmodbus
• ./configure ... --enable-non-distributable (tests/halcompile) - needed a missing include

[rene-dev]

I see what you did there, but this doesn't seem to be the best way of doing it.
how does the test need libmodbus when it cant talk to hardware?

[heeplr]

but this doesn't seem to be the best way of doing it

I briefly looked for a way with less side effects but couldn't find any. What would you suggest?

how does the test need libmodbus when it cant talk to hardware?

The issue is unrelated to hardware. mb2hal isn't built and attempting to load it results in the test failing with "mb2hal: no such file or directory".
Testing a configuration shouldn't rely on prerequisites of another configuration.

It's easily reproducible:

make distclean
./configure --without-libmodbus --with-realtime=uspace
make
../scripts/runtests -v ../tests/mb2hal

[petterreinholdtsen]

Please rebase on top of master to fix the build problem.

[andypugh]

Sorry, I have found myself on a train with time on my hands so I am looking at old PRs.

Is this still a live PR? Does it still seem like a good idea?

[heeplr]

works for me, so I can run tests in my sin-modbus and link-local-libreadline setup.

Using gnu find is certainly unconventional but I couldn't think of a cleaner way that doesn't introduce ongoing maintenance overhead.

[havardAasen]

Could it be an idea to documenting this behavior in file tests/md2hal/README?

I would also think that the skip files would make the git tree dirty, perhaps add these specific files to .gitignore?

[hansu]

Could it be an idea to documenting this behaviour in file tests/md2hal/README?

Yes, but it wouldn't help much and I think this is obviously that libmodus is needed for these tests.

I would also think that the skip files would make the git tree dirty, perhaps add these specific files to .gitignore?

That would be mandatory IMHO if we chose that way.

We could also add an option to the runtests script to exclude a folder.

[hansu]

I would be more specific to only delete the skip files that have been created by this script if we chose that option. So I would just add the skip file to the four tests that are affected even though this needs adaption if more mb2hal tests are added.

[havardAasen]

Could it be an idea to documenting this behaviour in file tests/md2hal/README?

Yes, but it wouldn't help much and I think this is obviously that libmodus is needed for these tests.

I might have been unclear on what I meant by documenting. I thought of documenting why and when the tests is being skipped, along with where the decision is made.

This will of course not help any test succeed, but any developer that is wondering why tests suddenly is skipped will have a easier time in understanding why.

[petterreinholdtsen]

Instead of adding and removing 'skip' files, why not make it executable and allow a more dynamic decision about enabling or skipping the test. The code in scripts/runtests.in look like this: if [ -e $testdir/skip ]; then if ! [ -x $testdir/skip ] || ! $testdir/skip; then echo "Skipping disabled test: $testdir" 1>&2 SKIP=$(($SKIP+1)) continue fi fi if $NOSUDO && [ -e $testdir/control ] && \ grep Restrictions: $testdir/control | grep -q sudo; then if ! [ -x $testdir/skip ] || ! $testdir/skip; then echo "Skipping sudo test: $testdir" 1>&2 SKIP=$(($SKIP+1)) continue fi fi As far as I can tell, it will run the script file if it is executable and skip the test if it fail.

…

-- Happy hacking Petter Reinholdtsen

[hansu]

But how do you want to prevent committing a file with changed permissions?

[petterreinholdtsen]

[Hans Unzner]

But how do you want to prevent committing a file with changed permissions?

Not sure I understand you. Normally one would not prevent it, just write some script into skip, do chmod a+x skip and commit the file. -- Happy hacking Petter Reinholdtsen

[hansu]

But the thing is that the modified or added skip file should not be committed because is should only control the execution of local tests if built e.g. without libmodbus.

[petterreinholdtsen]

[Hans Unzner]

But the thing is that the modified or added skip file should not be committed because is should only control the execution of local tests if built e.g. without libmodbus.

There should not be any need for locally modified 'skip' files, if they look something like this: #!/bin/sh if modbus_available; then exit 0 fi exit 1

…

-- Happy hacking Petter Reinholdtsen

[heeplr]

Afais there are 3 problems here:

• while disabling unworking tests in the configure phase will not have downsides, enabling them in configure might accidentally enable tests that are purposeful disabled temporarily which is afterwards overwritten when configure runs again
• we don't want script generated/deleted "skip" files accidentally committed
• we'd like to avoid maintenance overhead

We could also add an option to the runtests script to exclude a folder.

This seems the soundest solution to me, but would need manual bookkeeping of those in the configure context.

Maybe runtests could be modified to support some kind of lightweight "dependency management" by optionally allowing tests to define their prerequisites? This would still mean additional work but it's limited to the test itself and doesn't crosstalk into the configure context.

e.g.

        ...
        # skip test for unmet prerequisite(s)
        if [ -e "$testdir/musthave" ] ; then
            while IFS= read -r prereq ; do
                if ! grep --quiet --regexp "HAVE_$prereq yes" "$TOPDIR/src/config.h" ; then
                    echo "Skipping test for missing prerequisite \"$prereq\": $testdir" 1>&2
                    SKIP=$(($SKIP+1))
                    continue 2
                fi
            done < "$testdir/musthave"
        fi  
        # skip test if disabled by "skip" file
        if [ -e $testdir/skip ]; then
            ...

and then just add "musthave" files to each mb2hal test, containing "LIBMODBUS3" ...

As a bonus, each test depending on some HAVE_* feature could use that in the future.

[hansu]

@petterreinholdtsen How would you pass the modbus_available variable from the configure script to runtests?

[petterreinholdtsen]

[Hans Unzner]

@petterreinholdtsen How would you pass the modbus_available variable from the configure script to runtests?

I have not looked at the details, and thus do not have any concrete proposal how configure can pass this on. I guess some build state file (config.sub, config.h or similar), looking in PATH or checking for some files on disk might work. As configure know it, and the skip script need to know it, someone just need to make up some communication channel. :)

…

-- Happy hacking Petter Reinholdtsen

[hansu]

[Petter]

I guess some build state file (config.sub, config.h or similar), looking in PATH or checking for some files on disk might work. As configure know it, and the skip script need to know it, someone just need to make up some communication channel. :)

config.h looks suitable

If libmodbus is not excluded from build we have that define:
#define HAVE_LIBMODBUS3 yes
(Otherwise the undef is just uncommented: /* #undef HAVE_LIBMODBUS3 */)

So the mb2hal tests could look into config.h and run the test only if HAVE_LIBMODBUS3 is defined.

The tests could evaluate the output of
cpp -dM config.h | grep HAVE_LIBMODBUS3

[hansu]

Adding an executable skip file to each of the mb2hal test with the following content works like a charm for me:

#!/bin/sh
if cpp -dM config.h | grep -q "HAVE_LIBMODBUS3"; then
   exit 0
fi
exit 1

Thanks @petterreinholdtsen for that idea!
I think in general this is a fairly good solution. Any objections?

[heeplr]

@hansu

The tests could evaluate the output of cpp -dM config.h | grep HAVE_LIBMODBUS3

Couldn't $CXX be overridden for building and "cpp" (or "cxx") not actually in PATH? Extracting CXX from the Makefile certainly complicates things further.

What's wrong with the grep --quiet --regexp "HAVE_$prereq yes" "$TOPDIR/src/config.h" i suggested above?

Also for the sake of complexity, I'd not impose the check on the test-developer.
Introducing something simpler like "musthave" with one prerequisite per line, would work similar to the mechanism of "skip" and "expected".

[hansu]

Yeah we could also use grep --quiet --regexp "HAVE_$prereq yes" "$TOPDIR/src/config.h" instead of invoking cpp.
I only thought invoking cpp is safer because grep would also find that string inside comments.

Adding extra musthave files is too much complexity for those few cases I think.

[heeplr]

Yeah we could also use grep --quiet --regexp "HAVE_$prereq yes" "$TOPDIR/src/config.h" instead of invoking cpp. I only thought invoking cpp is safer because grep would also find that string inside comments.

That's right, the regexp should probably be ```"^#define HAVE_$prereq." for a more conservative match. (Also not every HAVE_ definition in config.h has a "yes")

Adding extra musthave files is too much complexity for those few cases I think.

It would avoid duplicate code (and possibilities for bugs). And I guess some existing (and not yet existing) tests could use such a feature aswell.

[heeplr]

I just pushed a new proposal. Definetly a cleaner attempt than wildly generating/deleting "skip" files. ;)

[hansu]

It would avoid duplicate code (and possibilities for bugs).

That's a point, yes