<div dir="ltr"><div>Hi Panos,<br></div><div><br></div><div>That sounds good, please see the details inline.<br></div><div><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Mar 29, 2019 at 8:27 AM Panagiotis Mavrogiorgos <<a href="mailto:pmav99@gmail.com">pmav99@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br></div><div>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 (<a href="http://fatra.cnr.ncsu.edu/grassgistests/reports_for_date-2019-03-26-07-00/report_for_nc_spm_full_v2alpha_nc/testfiles.html" target="_blank">example</a>).</div></div></div></div></blockquote><div><br></div><div>That's correct.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div></div><div>IMHV this practise is causing several issues:</div></div></div></div></blockquote><div><br></div><div>It is really a legacy which should removed as you suggest. The Bash-based tests are also bad for MS Win.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br></div><div>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 <a href="https://gist.github.com/pmav99/8e016e7c044555cef4e37d685a6622b6" target="_blank">95+ tests</a> written as shell scripts. I suspect that the testrunner is only matching "testsuite" directories while skipping directories named "tests", "test_suite" etc.</div><div><br></div><div>1a. There should be uniformity in the naming conventions of test directories and test files.</div><div>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. </div><div><br></div><div>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.</div></div></div></div></blockquote><div><br></div><div><div><br></div><div>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).<br></div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div><br></div><div>2a. Ideally and as a mid-term goal these tests should be converted to proper python tests</div><div>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:</div><div><a href="http://redsymbol.net/articles/unofficial-bash-strict-mode/" target="_blank">http://redsymbol.net/articles/unofficial-bash-strict-mode/</a><br><a href="https://disconnected.systems/blog/another-bash-strict-mode/" target="_blank">https://disconnected.systems/blog/another-bash-strict-mode/</a></div></div></div></div></blockquote><div><br></div><div>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).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>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:</div><div><br></div></div></div><blockquote style="margin:0px 0px 0px 40px;border:medium none;padding:0px"><div dir="ltr"><div dir="ltr"><div><font face="monospace, monospace">subprocess.check_out(shlex.split("bash test_foo.sh"))</font></div><div><font face="monospace, monospace"><br></font></div></div></div></blockquote><font face="arial, helvetica, sans-serif">This way, we would have reports with proper count of testsuites, failures etc.</font></div></blockquote><div><br></div><div><br></div><div>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: <br></div><div>
<p>test.t.vect.observe.strds succeeded unknown unknown 0 unknown<br>
test.t.rast.accdetect FAILED unknown unknown 0 unknown <br></p><p>to:<br></p><p>test.t.vect.observe.strds succeeded 1 1 0 100%<br>
test.t.rast.accdetect FAILED 1 0 1 0%</p><p>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.<br></p><p></p><p>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.<br></p>Summary 233 test files 93%1841 1801 24 98%</div><div><br></div><div>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%").</div><div><br></div><div>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.<br></div><div><br></div><div></div><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><font face="arial, helvetica, sans-serif"><br></font></div><div><font face="arial, helvetica, sans-serif">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.</font></div></div></blockquote><div><br></div><div>I agree that we need an official rule in the code submission guidelines.</div><div><br></div><div>Best,</div><div>Vaclav<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><font face="arial, helvetica, sans-serif"><br></font></div><div><font face="arial, helvetica, sans-serif">with kind regards,</font></div><div><font face="arial, helvetica, sans-serif">Panos</font></div></div>
_______________________________________________<br>
grass-dev mailing list<br>
<a href="mailto:grass-dev@lists.osgeo.org" target="_blank">grass-dev@lists.osgeo.org</a><br>
<a href="https://lists.osgeo.org/mailman/listinfo/grass-dev" rel="noreferrer" target="_blank">https://lists.osgeo.org/mailman/listinfo/grass-dev</a></blockquote></div></div>