Bring test suite back to a working state
Closed this issue · 12 comments
As it turns out, lslint has built-in support for unit testing. There is a csh script, test.csh, that is working. I've converted it to the more ubiquitous and well understood Bourne shell (sh) format, and I've added the -# parameter in the output:
#!/bin/sh
failed=.
if [ -e ./test.total.txt ] ; then
rm -f ./test.total.txt
fi
for f in scripts/*.lsl scripts/*/*.lsl ; do
printf "%40s ... " "$f"
./lslint -\# -A "$f" > ./test.run.txt 2>&1
if [ $? != 0 ] ; then
echo "FAILED"
echo "" >> ./test.total.txt
echo "****************" >> ./test.total.txt
echo '***>' "$f" >> ./test.total.txt
echo "" >> ./test.total.txt
cat ./test.run.txt >> ./test.total.txt
failed=$failed.
else
echo "passed"
fi
done
rm -f ./test.run.txt
if [ $failed != . ] ; then
cat ./test.total.txt
rm -f ./test.total.txt
fiHowever, now it needs to be reorganized to allow for Mono-only tests and LSO-only tests. I guess that specially named directories may do the job.
As of this writing, the test suite fails a lot of tests:
scripts/camera_demo.lsl ... FAILED
scripts/color-change.lsl ... FAILED
scripts/constants.lsl ... FAILED
scripts/constprop.lsl ... FAILED
scripts/error1.lsl ... FAILED
scripts/events.lsl ... FAILED
scripts/fpinc.lsl ... passed
scripts/fwcheck1.lsl ... passed
scripts/hex.lsl ... passed
scripts/irc-4.lsl ... FAILED
scripts/libhttpdb.lsl ... FAILED
scripts/mms_player.lsl ... FAILED
scripts/parserstackdepth.lsl ... FAILED
scripts/parserstackdepth2.lsl ... FAILED
scripts/parserstackdepth3.lsl ... FAILED
scripts/retval.lsl ... FAILED
scripts/scope1.lsl ... passed
scripts/scope2.lsl ... passed
scripts/scope3.lsl ... passed
scripts/streamcomment.lsl ... FAILED
scripts/test1.lsl ... passed
scripts/test2.lsl ... passed
scripts/unixtime.lsl ... passed
scripts/vconst.lsl ... passed
scripts/warning1.lsl ... FAILED
scripts/warning2.lsl ... FAILED
scripts/xytext1.2.lsl ... FAILED
scripts/apotheus_vendor/main.lsl ... FAILED
scripts/bugs/0001.lsl ... passed
scripts/bugs/0002.lsl ... passed
scripts/bugs/0003.lsl ... passed
scripts/bugs/0004.lsl ... passed
scripts/bugs/0005.lsl ... passed
scripts/bugs/0006.lsl ... FAILED
scripts/bugs/0007.lsl ... FAILED
scripts/bugs/0008.lsl ... FAILED
scripts/bugs/0009.lsl ... FAILED
scripts/bugs/0010.lsl ... FAILED
scripts/bugs/0011.lsl ... FAILED
scripts/bugs/0012.lsl ... passed
scripts/bugs/0013.lsl ... passed
scripts/bugs/0014.lsl ... passed
scripts/bugs/0015.lsl ... passed
scripts/bugs/0016.lsl ... passed
scripts/bugs/0017.lsl ... passed
scripts/bugs/0018.lsl ... passed
scripts/bugs/0019.lsl ... passed
scripts/bugs/0020.lsl ... passed
scripts/bugs/0021.lsl ... passed
scripts/bugs/0022.lsl ... FAILED
scripts/tltp/browser.lsl ... FAILED
scripts/tltp/exporter.lsl ... FAILED
scripts/tltp/server.lsl ... FAILED
That's 28 failures and 25 successes. Some of these are warnings or errors that have changed their number, because we (or at least I) have recklessly added or removed entries from the enum, without realizing that that would influence the test suite.
Others are due to testing a behaviour that is no longer current (like the infamous 3-2 bug that was fixed long ago).
At least one that I've found, is a failure to perform error recovery in the same way as before, and now it skips a whole list where it only skipped one element in past when finding an error. Cause is still unknown.
And then it's still possible that there are new bugs.
It would be ideal to bring the test suite back to a working state, and then add it to Travis CI so that we can see if a new change breaks something.
That is going to be a lot of work. I'll help with whatever I can, as time permits.
I've taken a first look at this. The initial commit by pclewis (after fixing the compile errors due to missing string.h), passes most tests, but in my system it aborts with failed assertions in 5 tests. I haven't looked into what these assertions are. (EDIT: Done now, it was a buffer overrun in NULL_KEY in the lexer. Once fixed, all tests passed. That explains that the next commit fixed it, because the constants were moved out of the lexer.)
The next commit by Adam Wozniak doesn't change anything (it only makes it compile again and adds the clean target to the makefile). The next after that (24127a6) fixes the assertion errors somehow, and these 5 test pass. It introduces constants from builtins.txt, but in doing so, it fails to add their values, causing two different tests to fail (the other errors are fixed by updating builtins.txt). Something else seems broken. Will investigate. (EDIT: Definitely all errors in that commit were due to constants not having predefined values.)
For reference, here is how the test suite works. Option -A turns lslint into its own unit test program, as follows:
- If a line has a comment like
// $[Ennnnn](for some number nnnnn, where 10000-19999 are errors are 20000-29999 are warnings), then it indicates that an error or warning is expected at that line. - If a line generates no errors or warnings, and has no such comment, then nothing happens for that line.
- If a line would normally generate a warning or error without
-A, and the code for that error or warning matches the one in the$[Ennnnn]comment, then nothing happens for that line either. - If a line would normally generate an error or warning without
-A, but it has no such comment, then an error is emitted like "Unexpected warning" or "Unexpected error" as appropriate. - If a line does not generate an error or warning, but it has such a comment, it shows an "Assertion error" with the expected error or warning code that didn't happen.
- If there is an error or warning but it does not match, then both the unexpected and the assertion error are emitted.
- Some constructs generate several errors or warnings. For that,
// $[Ennnnn] $[Emmmmm] ...can be specified. - At the end, the total is printed with how many assertion errors or unexpected messages were emitted.
- Option
-#is quite useful in creating test cases because it displays the error numbers. It can be added to-Afor extra help. I don't know why it's not part of the testing script because it's very helpful.
Locally, I've managed to reduce the test suite to two failed tests. These are genuine regressions.
One of them is that #6 is not fully fixed: global vectors and rotations can't have negative components. I have a plan to fix it for good, and hopefully completely eliminate the shift/reduce and reduce/reduce conflicts in the grammar.
The other is that the value of pseudo-constants (globals that are never written) is not properly detected if they come from a constant, for example:
integer face = ALL_SIDES;
default{timer(){
if (face == ALL_SIDES) // expected $[E20011] (always true) but nothing happens
0;
}}I believe this is an instance of a more general problem:
integer A = 1;
integer B = A;
default{timer(){
if (B == 1) // expected $[E20011] but nothing happens
0;
}}The first case showed the warning in past, because constants were converted to their value at parse time, so it was actually an instance of this case, which does work as intended:
integer A = 1;
default{timer(){
if (A == 1) // $[E20011] always true
0;
}}But when it was modified so that the constants were read from builtins.txt instead, that turned the first case into an instance of the second case, rather than the third.
#21 fixes the last two tests. I've introduced a make check target that runs the test suite for non-Windows machines. It would be awesome if the Travis build included that, so that we can see if the tests pass on every commit and PR, to avoid in future having a situation similar to the one before this bug.
Currently there is a lot of noise in the syntax test files. A focus on minimal working examples would be greatly appreciated.
The unit testing suite? It's for testing the internal self-consistency of lslint, mainly to ensure that changes don't break existing behaviour. It doesn't matter much how long or short or clear or convoluted they are; what matters is why a test fails in case one does.
If you want examples of the use of lslint, that's not what these are.
By the way, the reason this is still open is because of the pending decision of whether to include the tests in Travis or not.
I have some test files for using LSL with Sublime Text 3:
⚠️ I mainly test words there. A full test for the grammar will be added later.
@ebickle has a single, more grammar-related file:
https://github.com/ebickle/lsl-vscode/blob/master/test/samples/syntax-test-1.lsl
I think you're completely missing the purpose of the scripts/ directory.
It's designed to test the internal consistency of lslint's features. See https://en.wikipedia.org/wiki/Unit_testing (especially Advantages). Two bugs surfaced as a result of examining the test suite after it was repaired; see my 4th comment.
Each test is designed to trigger a certain feature of lslint, to ensure it's working as intended, and if it fails for some reason, to alert the programmer.
Running the unit testing suite (test.sh) currently produces this output:
scripts/camera_demo.lsl ... passed
scripts/color-change.lsl ... passed
scripts/comparison.lsl ... passed
scripts/constants.lsl ... passed
scripts/constprop.lsl ... passed
scripts/deprecated.lsl ... passed
scripts/events.lsl ... passed
scripts/fpinc.lsl ... passed
scripts/fwcheck1.lsl ... passed
scripts/globals.lsl ... passed
scripts/hex.lsl ... passed
scripts/irc-4.lsl ... passed
scripts/libhttpdb.lsl ... passed
scripts/mms_player.lsl ... passed
scripts/parserstackdepth.lsl ... passed
scripts/parserstackdepth2.lsl ... passed
scripts/parserstackdepth3.lsl ... passed
scripts/print.lsl ... passed
scripts/retval.lsl ... passed
scripts/scope1.lsl ... passed
scripts/scope2.lsl ... passed
scripts/scope3.lsl ... passed
scripts/streamcomment.lsl ... passed
scripts/test1.lsl ... passed
scripts/test2.lsl ... passed
scripts/unixtime.lsl ... passed
scripts/vconst.lsl ... passed
scripts/warning1.lsl ... passed
scripts/warning2.lsl ... passed
scripts/xytext1.2.lsl ... passed
scripts/apotheus_vendor/main.lsl ... passed
scripts/bugs/0001.lsl ... passed
scripts/bugs/0002.lsl ... passed
scripts/bugs/0003.lsl ... passed
scripts/bugs/0004.lsl ... passed
scripts/bugs/0005.lsl ... passed
scripts/bugs/0006.lsl ... passed
scripts/bugs/0007.lsl ... passed
scripts/bugs/0008.lsl ... passed
scripts/bugs/0009.lsl ... passed
scripts/bugs/0010.lsl ... passed
scripts/bugs/0011.lsl ... passed
scripts/bugs/0012.lsl ... passed
scripts/bugs/0013.lsl ... passed
scripts/bugs/0014.lsl ... passed
scripts/bugs/0015.lsl ... passed
scripts/bugs/0016.lsl ... passed
scripts/bugs/0017.lsl ... passed
scripts/bugs/0018.lsl ... passed
scripts/bugs/0019.lsl ... passed
scripts/bugs/0020.lsl ... passed
scripts/bugs/0021.lsl ... passed
scripts/bugs/0022.lsl ... passed
scripts/fn_overr/override.lsl ... passed
scripts/godmode/godmode.lsl ... passed
scripts/lazylist/lazylists.lsl ... passed
scripts/lso/comparison.lsl ... passed
scripts/lso/error1.lsl ... passed
scripts/lso/jump.lsl ... passed
scripts/lz_overr/lazy_with_override.lsl ... passed
scripts/mono/comparison.lsl ... passed
scripts/mono/error1.lsl ... passed
scripts/mono/jump.lsl ... passed
scripts/preproc/ignore.lsl ... passed
scripts/switch/switch.lsl ... passed
scripts/tltp/browser.lsl ... passed
scripts/tltp/exporter.lsl ... passed
scripts/tltp/server.lsl ... passed
scripts/todo/components.lsl ... passed
scripts/todo/event-names.lsl ... passed
scripts/uep/events.lsl ... passed
Note in particular the directory bugs/ which probably alludes to issues from the original google code bug tracker. These bugs were found and reported by users, and test cases that exercised the fix were added to the test suite, to ensure that future changes won't break them again.
Some scripts with a purpose other than testing lslint (e.g. camera_demo.lsl) are probably scripts donated by users to demonstrate lslint bugs, and they were probably incorporated to the test suite with permission from their creators. They are not intended as demos of lslint or its features; they are intended as tests that exercise a part of the code that was not working properly. Rather than isolating the problem into a short test case demonstrating it, the full script was included, possibly because of laziness or lack of time by either the author of the script or the author of lslint.
Most or all of the grammar's features are tested in the test suite already. Except for a part that I rewrote, related to the values of globals, for which I added extensive testing, the grammar file has been taken directly from LSL's parser and it just got added a few features. The grammar should work, because it works for LSL. The lslint-specific features of the grammar have their tests.
In short, it doesn't make sense to suggest the addition or replacement of test files unless a bug is found, or new code is written and needs a test to demonstrate it works properly and ensure it doesn't get broken in future, or code is found that isn't being run by the current test suite and can be exercised with an extra test.
So, if you find a bug and can provide a test case, that test case can be incorporated to the test suite. If you write a new feature for lslint, a test case is welcome, not to say mandatory, that demonstrates it's working. If you run a C++ code coverage tool on lslint that discovers an area that isn't exercised by the test suite, and you can come up with a LSL program that exercises it, then that is worth incorporating.
There's no other reason to replace, add, or remove scripts from the test suite.
@Sei-Lisa running lslint on my test files, I found kwdb is missing these:
- OBJECT_RETURN_TIME
- OBJECT_REZ_TIME
- OBJECT_SAT_UPON
- OBJECT_SELECTED
http://wiki.secondlife.com/wiki/Release_Notes/Second_Life_Server/17#17.08.11.328159
Running lslint on the other single file I linked above, gives me correct warnings and errors, but they're mostly displaced.
@Sei-Lisa running lslint on my test files, I found kwdb is missing these:
No it's not, and this isn't the forum to discuss that anyway. It may be that lslint hasn't been updated to the latest kwdb, but that's material for a new issue, not for hijacking an existing one.
Running lslint on the other single file I linked above, gives me correct warnings and errors, but they're mostly displaced.
Again, this isn't the place to discuss that. Please report this as a new issue, including the output of lslint rather than a picture. This one is about problems in the test suite. It is now in working order, and as I said above, this issue only remains open so that @Makopo considers whether to include running the test suite in Travis.