[GRASS-dev] Shell scripts for tests

[Vaclav Petras]

Hi Panos,

That sounds good, please see the details inline.

On Fri, Mar 29, 2019 at 8:27 AM Panagiotis Mavrogiorgos <pmav99 at gmail.com>
wrote:

>
> The GRASS testsuite contains several tests that have been written as shell
> scripts. If I am not mistaken, those can be identified by the "unknown"
> percentages in the testreport (example
> <http://fatra.cnr.ncsu.edu/grassgistests/reports_for_date-2019-03-26-07-00/report_for_nc_spm_full_v2alpha_nc/testfiles.html>
> ).
>

That's correct.


> IMHV this practise is causing several issues:
>

It is really a legacy which should removed as you suggest. The Bash-based
tests are also bad for MS Win.


>
> 1. The testrunner seems to be skipping/omitting the majority of the
> available tests. More specifically, the testrunner currently runs 27 shell
> scripts, while it seems that there are 95+ tests
> <https://gist.github.com/pmav99/8e016e7c044555cef4e37d685a6622b6> written
> as shell scripts. I suspect that the testrunner is only matching
> "testsuite" directories while skipping directories named "tests",
> "test_suite" etc.
>
> 1a. There should be uniformity in the naming conventions of test
> directories and test files.
> 1b. Needless to say, we need to make sure that the testrunner runs all the
> available tests. I guess that if 1a is taken care off then 1b will have
> been resolved too.
>
> 2. I only checked a bunch of the shell scripts but I am under the
> impression that majority of them (if not all!) are not real tests but just
> shell scripts that prove that the code runs while not really asserting that
> the code is actually correct. Of course, this is better than nothing, but
> it is not good enough.
>


To both 1 and 2: The names which are not "testsuite" are tests which are
not ready or were not checked by anybody as suitable to run automatically.
So the naming is actually to distinguish the legacy tests which still need
to be updated. There can be several reasons, for example, they don't
actually report that anything happen as you note in 2b (or perhaps they do
something which the current runner does not expect - I don't know if this
is actually there, but it's possible).


>
> 2a. Ideally and as a mid-term goal these tests should be converted to
> proper python tests
> 2b. In the meantime, and if it is not already being used, we should try to
> make sure that the shells scripts are using "strict mode" so that errors do
> propagate:
> http://redsymbol.net/articles/unofficial-bash-strict-mode/
> https://disconnected.systems/blog/another-bash-strict-mode/
>

I agree. I think the test runner actually runs them at least in some strict
mode already (or at least it could; if I recall, it is using at least -x
now).


> 2c. Also in the meantime, and in order to make testreports more useful, we
> could also add a single python test file next to each shell script which
> will contain something similar to this:
>
> subprocess.check_out(shlex.split("bash test_foo.sh"))
>
> This way, we would have reports with proper count of testsuites, failures
> etc.
>


The Bash tests are already reporting "succeeded" and "FAILED" as do the
Python ones for the whole file. Maybe I'm missing something here, but the
what you are suggesting would change the current:

test.t.vect.observe.strds succeeded unknown unknown 0 unknown
test.t.rast.accdetect FAILED unknown unknown 0 unknown

to:

test.t.vect.observe.strds succeeded 1 1 0 100%
test.t.rast.accdetect FAILED 1 0 1 0%

The last four columns deal with the individual tests within the file. The
Bash files are considered as not having any structure (or just one test),
so no stats for that.

There is a statistics for the success of whole files. In the following, it
is the number 93% computed from the "succeeded" and "FAILED" states.
Summary 233 test files 93%1841 1801 24 98%

If the "unknown unknown 0 unknown" bothers you and you want the stats to be
proper, rather than making Python wrappers, I suggest putting there the
proper values ("1 1 0 100%" and "1 0 1 0%").

I think the reason why I decided to put there the unknowns rather than 0s
and 1s is that there are multiple tests in one Bash file, so I was just
trying not to make false impression, but you can say that such file is
considered to be one test, whatever is in it, so than you can say one test
failed or succeeded.


> 3. Finally, IMHV there should also be an official policy/requirement, that
> new code/modules cannot be accepted unless they have proper and
> comprehensive tests written in python.
>

I agree that we need an official rule in the code submission guidelines.

Best,
Vaclav


>
> with kind regards,
> Panos
> _______________________________________________
> grass-dev mailing list
> grass-dev at lists.osgeo.org
> https://lists.osgeo.org/mailman/listinfo/grass-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/grass-dev/attachments/20190329/894eb6dd/attachment.html>