Automatic tests can succeed with syntax errors

Discussion
#1

The test suite for building SWI-Prolog is run by the command ctest, which uses the file packages/cmake/PrologPackage.cmake to run tests defined by

  add_test(NAME "${SWIPL_PKG}:${test_name}"
	   COMMAND ${PROG_SWIPL} -p "foreign=${pforeign}"
			 -f none --no-packs -s ${test_source}
			 -g "${test_goal}"
			 -t halt)

Unfortunately, this doesn’t catch syntax errors in the tests, as demonstrated here (outside the ctest framework, because I’m working in a separate module:

$ swipl "-p" "foreign=" "-f" "none" "--no-packs" "-s" test_protobufs.pl "-g" "test_protobufs" "-t" "halt" ; echo $?
ERROR: /home/peter/src/contrib-protobufs/test_protobufs.pl:267:22: Syntax error: Operator expected
% PL-Unit: protobuf_message ....... done
% PL-Unit: protobuf_segment_convert ....... done
% All 14 tests passed
0

Question: is there an option to swipl that will cause a failure if a file is loaded that has a syntax error?

By the way, here’s the same thing done using ctest, by introducing a deliberate syntax error into the current source (which doesn’t use plunit, so it produces different output):

$ ctest  -R protobufs
Test project /home/peter/src/swipl-devel/build
    Start 49: protobufs:protobufs
1/1 Test #49: protobufs:protobufs ..............   Passed    0.18 sec

100% tests passed, 0 tests failed out of 1

Total Test time (real) =   0.19 sec

$ ctest -V -R protobufs
UpdateCTestConfiguration  from :/home/peter/src/swipl-devel/build/DartConfiguration.tcl
UpdateCTestConfiguration  from :/home/peter/src/swipl-devel/build/DartConfiguration.tcl
Test project /home/peter/src/swipl-devel/build
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 49
    Start 49: protobufs:protobufs

49: Test command: /home/peter/src/swipl-devel/build/src/swipl "-p" "foreign=" "-f" "none" "--no-packs" "-s" "/home/peter/src/swipl-devel/packages/protobufs/test_protobufs.pl" "-g" "test_protobufs" "-t" "halt"
49: Test timeout computed to be: 10000000
49: ERROR: /home/peter/src/swipl-devel/packages/protobufs/test_protobufs.pl:308:5: Syntax error: Operator expected
49: Loading Google's Golden Wirestream (2.5.0)...OK
49: Unifying canned Golden Message with canned Golden Template...OK
49: Unifying canned Golden Message with Google's Golden Wirestream...OK
49: Parsing Google's Golden Wirestream to canned Golden Template...OK
49: Comparing canned Golden Message to parsed Golden Template...OK
49: Serializing canned Golden Message to Codes...OK
49: Comparing Google's Golden Wirestream to Codes...OK
49: Parsing Codes to canned Golden Template...OK
49: Comparing canned Golden Message to parsed Golden Template...OK
49: All tests passed.
1/1 Test #49: protobufs:protobufs ..............   Passed    0.18 sec

The following tests passed:
	protobufs:protobufs

100% tests passed, 0 tests failed out of 1

Total Test time (real) =   0.18 secc
Changes in error handling
#2

I have also been bitten by this behavior but in a server environment with automatic code deployment. Code was deployed and started successfully despite some modules having loading errors. The errors would cause issues later when specific code path was run.

I defined a dynamic flag “started” to counter this. The flag was set after the server was started. In your case you could set a flag right after loading but before running the tests. Any error or warning emitted before the flag is set would be considered as a loading error and would terminate the process with non-zero status code (make sure this hook comes before other code):

user:message_hook(Term, Type, _):-
    \+ started,
    ( Type = error ; Type = warning ),
    message_to_string(Term, String),
    writeln(user_error, String),
    halt(1).

In development you could disable the hook to make the “make”-based editing not exit the process.

#3

Probably the best way out would be to provide some library that you can load at the start of your program or even using a commandline argument. As compiling and running are not well separated stages in SWI-Prolog there are many complicated cases though. For tests even more as there are tests that validate that compiling some code produces some specific message … We probably need some flag that says “errors expected”

#4

It gets worse: a syntax error doesn’t result in an error from the compiler (swipl --stand_alone=true --undefined=error --verbose=false --foreign=save -o xxx.qlf -c xxx.pl).

I hadn’t noticed this before because a syntax error usually cascades to other errors that do stop the compiler, Why is syntax error special in not giving a bad return code? (For example, --undefined=error sets the return code the way I expect.)

(I tried @rla 's message_hook trick and it did stop a compilation with a return code, which is nice.)

#5
#6

I’m still pretty unsure on how to address this. Yes, it is a problem in unattended builds. A robust solution that will not cause basically ok programs that print error messages to exit with status 1 is far from trivial. One option might be to do this only for swipl -c file.pl as that is not supposed to run anything except stuff that supports the compilation process.

For general case it is mostly initialization goals that cause problems as these may support the compilation (should be considered an error) but may also run the application. Many people use initialization/1 to start applications. On SWI-Prolog that is not the best solution and one should use initialization/2 to be more precise on when the initialization hook is executed.

Another problem is what is an error? We could state any call to print_message/2 using error. However, some programs deliberately capture these messages with the purpose to suppress them. So, print_message/2 that is not successfully hooked? But then we have programs that hook to redirect messages to some channel such as a logging facility.

This seems to be the price of integrating an incremental compiler into the runtime environment and allow running arbitrary goals as a side effect of compiling a file.

#7

This (partial) solution would at least produce “less surprise” when generating .qlf files in Makefiles. Perhaps controlled by a flag --error=error (to match --undefined=error)? I presume that if someone wants to suppress a specific error, they can use the user:message_hook/2 trick.
(Might also want a --warning=error flag, similar to g++'s -Wall).

Question: does the user:mesage_hook/2 trick work across all imported files by just defining user:message_hook/2 in the main module, or would I have to add it to each imported file? (e.g., to avoid the problem that @rla described)

#8

Where in the source is the compiler called? (I tried tracing through the top-level intialization, but I got lost).

Would I be better off by creating a user:message/3 hook, and then running qsave_program/2, rather than using swipl -c? But I’d need to somehow remove the user:message/3 hook from the saved state …

#9

The -c invocation is handled by ‘$compile’/0 in boot/toplevel.pl. One solution to get rid of the hook is initialization/2 with When set to restore_state. Alternatively just before creating the state. Not sure anything can go wrong that doesn’t lead to failure anyway. Or create a generic handler that always fails except while creating the state.

#10

That sounds like a non-trivial amount of work, so I guess we’ll just have to leave this as an open issue in github.

For my purposes, it’s sufficient to have a user:message_hook/3 that I can abolish after loading the saved state - and it appears that user:message_hook/3 has global effect, so it seems that I can trap syntax errors with a single hook definition.

For now, I’ll just put this into my code (it’s probably insufficient - e.g., doesn’t handle a missing fie or some other “consult” errors):

user:message_hook(Term, error, Lines) :-
    Term = error(syntax_error(_Msg), _Details),
    print_message_lines(user_error, 'ERROR: ', Lines),
    halt(1).
#11

It seems that there’s a solution, and a rather elegant one at that. Thank-you @jan!

1 Like