New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP, RFC] Move regression tests out from the main code base #593

Merged
merged 3 commits into from Oct 12, 2017

Conversation

Projects
None yet
4 participants
@busstoptaktik
Member

busstoptaktik commented Oct 8, 2017

18 months ago, @kbevers and I introduced a large number of regression tests in PROJ.4, at once rising the test coverage from (iirc) around 30% to around 70%.

Now, I’d like to get rid of those tests. Or rather, I’d like to move them out of the central code base, where they weigh thousands of lines of code.

They were introduced because we were about to do some huge refactorings, making it quite important to make sure we got the forwards, inverses, destructors, sphericals and ellipsoidals of each projection back in the right place after having thrown all the code up in the air.

In hindsight, we could however, have done the job in a cleaner way, especially given that our mission was to make the code more maintainable.

To repair on this, I now propose a test file builtins.gie and a new test environment, the Geospatial Integrity Investigation Environment, gie (evidently named in honour of PROJ.4 founder Gerald Ian Evenden, 1935--2016).

A gie test can be defined from 3 command verbs: OPERATION, which takes a +proj=... expression as input, ACCEPT, which takes a test point coordinate as input, and EXPECT, which defines the expected result of the coordinate operation.

I wrote gie partially because I need it for some PROJ.4 related work I’m heading, under the auspicies of the Nordic Geodetic Commision, where we study the use of PROJ.4 as a replacement for nationally developed transformation systems (perhaps “promote” is actually the right word, but “study” is what went into our terms of reference).

In that connection, gie is intended as a tool to extract point reference material from geodetic organisations populated by people of various backgrounds, of which some are neither used to C code, nor OGC standards.

gie requires no other tooling than the C compiler already used for compiling the library. Hence, it can run on any platform on which PROJ.4 works. It already replicates the built in regression tests (in the builtins.gie file), and it appears that a large part of the Unix shell based testing in the nad directory, can be converted to run under gie as well. This would greatly increase the amount of testing that can be done for the AppVeyor CI targets.

Hence, I propose we introduce gie as part of the PROJ.4 package, partially for our own integration and regression testing, partially to provide an interface for external people to contribute to PROJ.4 by submitting additional test material in a simple way.

If you agree, I will go on removing from the C code, the regression testing, that has already been replicated in builtins.gie.

For your information, here is the initial remark in gie.c. Also see the description in builtins.gie:

/***********************************************************************

       gie - The Geospatial Integrity Investigation Environment

************************************************************************

The Geospatial Integrity Investigation Environment "gie" is a modest
regression testing environment for the PROJ.4 transformation library.

Its primary design goal was to be able to replace those thousands of
lines of regression testing code that are (at time of writing) part
of PROJ.4, while not requiring any other kind of tooling than the same
C compiler already employed for compiling the library.

The basic functionality of the gie command language is implemented
through just 3 command verbs:

OPERATION,     which defines the PROJ.4 operation to test,
ACCEPT,        which defines the input coordinate to read, and
EXPECT,        which defines the result to expect.

E.g:

operation  +proj=utm  +zone=32  +ellps=GRS80
accept     12  55
expect     691_875.632_14   6_098_907.825_05

Note that gie accepts the underscore ("_") as a thousands separator.
It is not required (in fact, it is entirely ignored by the input
routine), but it significantly improves the readability of the very
long strings of numbers typically required in projected coordinates.

By default, gie considers the EXPECTation met, if the result comes to
within 0.5 mm of the expected. This default can be changed using the
TOLERANCE command verb (and yes, I know, linguistically speaking, both
"operation" and "tolerance" are nouns, not verbs). See the first
few hundred lines of the "builtins.gie" test file for more details of
the command verbs available (verbs of both the VERBal and NOUNistic
persuation).

--

But more importantly than being an acronym for "Geospatial Integrity
Investigation Environment", gie were also the initials, user id, and
USGS email address of Gerald Ian Evenden (1935--2016), the geospatial
visionary, who, already in the 1980s, started what was to become the
PROJ.4 of today.

Gerald's clear vision was that map projections are *just special
functions*. Some of them rather complex, most of them of two variables,
but all of them *just special functions*, and not particularly more
special than the sin(), cos(), tan(), and hypot() already available in
the C standard library.

And hence, *they should not be particularly much harder to use*, for a
programmer, than the sin()s, tan()s and hypot()s so readily available.

Gerald's ingenuity also showed in the implementation of the vision,
where he devised a highly comprehensible, yet simple, system of key-value
pairs for parameterising a map projection, and the highly flexible
PJ struct, storing run-time compiled versions of those key-value pairs,
hence making a map projection function call, pj_fwd(PJ, point), as easy
as a traditional function call like hypot(x,y).

While today, we may have more formally well defined metadata systems
(most prominent the OGC WKT representation), nothing comes close being
as easily readable ("human compatible") as Gerald's key-value system.
This system in particular, and the PROJ.4 system in general, was
Gerald's great gift to anyone using and/or communicating about geodata.

It is only reasonable to name a program keeping an eye on the integrity
of the PROJ.4 system in honour of Gerald. So in honour, and hopefully
also in the spirit, of Gerald Ian Evenden (1935--2016), this is the
Geospatial Integrity Investigation Environment.

************************************************************************

Thomas Knudsen, thokn@sdfe.dk, 2017-10-01/2017-10-08

************************************************************************

* Copyright (c) 2017 Thomas Knudsen
* Copyright (c) 2017, SDFE
*
* Permission is hereby granted, free of charge, to any person obtaining a
* copy of this software and associated documentation files (the "Software"),
* to deal in the Software without restriction, including without limitation
* the rights to use, copy, modify, merge, publish, distribute, sublicense,
* and/or sell copies of the Software, and to permit persons to whom the
* Software is furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included
* in all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
* OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
* THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
* DEALINGS IN THE SOFTWARE.

***********************************************************************/

@busstoptaktik busstoptaktik requested review from hobu, rouault and kbevers and removed request for hobu, rouault and kbevers Oct 8, 2017

@busstoptaktik

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Oct 8, 2017

Member

@hobu @kbevers @rouault - sorry guys: For a few hours, you were ticked off in the "request a review" Github menu. The code in this PR is nowhere near ready for that. What I meant to do was asking for your comments, which is probably better done by @ mentioning you here

Member

busstoptaktik commented Oct 8, 2017

@hobu @kbevers @rouault - sorry guys: For a few hours, you were ticked off in the "request a review" Github menu. The code in this PR is nowhere near ready for that. What I meant to do was asking for your comments, which is probably better done by @ mentioning you here

@rouault

This comment has been minimized.

Show comment
Hide comment
@rouault

rouault Oct 8, 2017

Member

Before removing the existing tests, some care should be taken that they are transposed to the new framework. In particular, looking at the current state of gie which uses proj_create(), I'm wondering how you would transfer test cases of the form +proj= +to +proj= since some tests like in testdatumfile involve a precise source and target SRS, and it would be good to keep them on that form as I guess that some test points might be reference points that can be found in external documents.
Some of the existing test files are also run conditionnaly if some grids are present or not (see check-local target of nad/Makefile.am)
We might still want to keep a simple shell script based test to ensure that cs2cs and proj, as binaries, are still tested. No need to test all the projections (gie would test that), but more the switches of the programs.

Member

rouault commented Oct 8, 2017

Before removing the existing tests, some care should be taken that they are transposed to the new framework. In particular, looking at the current state of gie which uses proj_create(), I'm wondering how you would transfer test cases of the form +proj= +to +proj= since some tests like in testdatumfile involve a precise source and target SRS, and it would be good to keep them on that form as I guess that some test points might be reference points that can be found in external documents.
Some of the existing test files are also run conditionnaly if some grids are present or not (see check-local target of nad/Makefile.am)
We might still want to keep a simple shell script based test to ensure that cs2cs and proj, as binaries, are still tested. No need to test all the projections (gie would test that), but more the switches of the programs.

@busstoptaktik

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Oct 8, 2017

Member

Before removing the existing tests, some care should be taken that they are transposed to the new framework.

Definitely - and I do not necessarily advocate removing the existing shell script based tests. My main intention really is to reduce the size and noise level of the primary code base by moving material that does not need to be int the src/ tree into the test/ tree.

In particular, looking at the current state of gie which uses proj_create(), I'm wondering how you would transfer test cases of the form +proj= +to +proj=

Either not at all (i.e. keeping them "shell script only"), or by detecting +to and call pj_transform() (or proj_create_crs_to_crs() before proj_trans_coord()). Preferably pj_transform(), since @warmerdam has probably handled a lot of corner cases there, which have not yet been triggered in the proj_...() API.

since some tests like in testdatumfile involve a precise source and target SRS, and it would be good to keep them on that form as I guess that some test points might be reference points that can be found in external documents.

Fully agree - providing good test material is hard, and we should not get rid of anything we have.

We might still want to keep a simple shell script based test to ensure that cs2cs and proj, as binaries, are still tested. No need to test all the projections (gie would test that), but more the switches of the programs.

I definitely agree: I have no intention of replacing the existing test system. I just want to extend it and make more testing available on Windows and other exotic platforms, without cluttering the primary code base.

Member

busstoptaktik commented Oct 8, 2017

Before removing the existing tests, some care should be taken that they are transposed to the new framework.

Definitely - and I do not necessarily advocate removing the existing shell script based tests. My main intention really is to reduce the size and noise level of the primary code base by moving material that does not need to be int the src/ tree into the test/ tree.

In particular, looking at the current state of gie which uses proj_create(), I'm wondering how you would transfer test cases of the form +proj= +to +proj=

Either not at all (i.e. keeping them "shell script only"), or by detecting +to and call pj_transform() (or proj_create_crs_to_crs() before proj_trans_coord()). Preferably pj_transform(), since @warmerdam has probably handled a lot of corner cases there, which have not yet been triggered in the proj_...() API.

since some tests like in testdatumfile involve a precise source and target SRS, and it would be good to keep them on that form as I guess that some test points might be reference points that can be found in external documents.

Fully agree - providing good test material is hard, and we should not get rid of anything we have.

We might still want to keep a simple shell script based test to ensure that cs2cs and proj, as binaries, are still tested. No need to test all the projections (gie would test that), but more the switches of the programs.

I definitely agree: I have no intention of replacing the existing test system. I just want to extend it and make more testing available on Windows and other exotic platforms, without cluttering the primary code base.

@busstoptaktik

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Oct 8, 2017

Member

@cffk: in the gieprogram, in this PR, I use geod_inverse() (through proj_lp_dist()) to compute distances between points that may occasionally be just micrometres from each other, and test it against another micrometre level threshold. I agree that it is unusual - but it is also very convenient . So I would like to hear your opinion: Do you see any problems with this?

Member

busstoptaktik commented Oct 8, 2017

@cffk: in the gieprogram, in this PR, I use geod_inverse() (through proj_lp_dist()) to compute distances between points that may occasionally be just micrometres from each other, and test it against another micrometre level threshold. I agree that it is unusual - but it is also very convenient . So I would like to hear your opinion: Do you see any problems with this?

@rouault

This comment has been minimized.

Show comment
Hide comment
@rouault

rouault Oct 8, 2017

Member

Definitely - and I do not necessarily advocate removing the existing shell script based tests.

OK, I wasn't really clear on the intended scope, but re-reading your proposal more carefuly, I see this is just about the code you added in the src/pj_ files. Sounds good to me.

Member

rouault commented Oct 8, 2017

Definitely - and I do not necessarily advocate removing the existing shell script based tests.

OK, I wasn't really clear on the intended scope, but re-reading your proposal more carefuly, I see this is just about the code you added in the src/pj_ files. Sounds good to me.

@cffk

This comment has been minimized.

Show comment
Hide comment
@cffk

cffk Oct 8, 2017

Contributor

@busstoptaktik The absolute error in the distance returned by geod_inverse is less than 15 nanometers. So this is a fine way to measure the separation of points micrometers apart.

Contributor

cffk commented Oct 8, 2017

@busstoptaktik The absolute error in the distance returned by geod_inverse is less than 15 nanometers. So this is a fine way to measure the separation of points micrometers apart.

@kbevers

kbevers approved these changes Oct 9, 2017

I am completely in favour of moving the regression tests out of the src/ tree. As far as I understand the code in this PR it takes care of all the tests that can be described in terms of a proj-string, a set of input coordinates and matching expected results. That takes care of most of the selftests in the PJ_*.c files. Actually it looks like you used that mechanism to create the builtins.gie file. A few thing is still not covered by this test setup if the selftest is removed from proj. Those are the API function tests (most notably in PJ_cart.c and tests that do not use pj_generic_selftest() (e.g. vgridshift and helmert). The last part fits in nicely in gie and the test data can be added manually, but the API function tests are not covered by gie (as of now, anyway). Do you have a plan for that? Maybe we should use an existing test framework for that?

Do you plan on distributing gie alongside the rest of the package, or is it only meant as a development tool? I ask because I noted that gie.c is placed src/ and not in test/gie/.

Show outdated Hide outdated test/gie/builtins.gie
Show outdated Hide outdated src/gie.c
int verbosity;
int nargs;
int op_id;
int op_ok, op_ko;

This comment has been minimized.

@kbevers

kbevers Oct 9, 2017

Member

does "ko" mean "the opposite of ok"?

@kbevers

kbevers Oct 9, 2017

Member

does "ko" mean "the opposite of ok"?

This comment has been minimized.

@busstoptaktik

busstoptaktik Oct 10, 2017

Member

yes - any better suggestions? I like it because it not only indicates an opposition, but also means "knock out" in pugilistic lingo - so if you run a test it is either OK or a knock out :-)

@busstoptaktik

busstoptaktik Oct 10, 2017

Member

yes - any better suggestions? I like it because it not only indicates an opposition, but also means "knock out" in pugilistic lingo - so if you run a test it is either OK or a knock out :-)

Show outdated Hide outdated src/gie.c
@cffk

This comment has been minimized.

Show comment
Hide comment
@cffk

cffk Oct 10, 2017

Contributor

Given that you're improving proj_stdtod.c, how about giving dmstor.c a once over? This has a serious misfeature that the hemisphere designators N,E,W,S, are only used to provide a sign and not to indicate, in addition, latitude vs longitude. Thus a user might specify 45E and have proj.4 treat it like 45N.

Contributor

cffk commented Oct 10, 2017

Given that you're improving proj_stdtod.c, how about giving dmstor.c a once over? This has a serious misfeature that the hemisphere designators N,E,W,S, are only used to provide a sign and not to indicate, in addition, latitude vs longitude. Thus a user might specify 45E and have proj.4 treat it like 45N.

@busstoptaktik

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Oct 10, 2017

Member

Given that you're improving proj_stdtod.c, how about giving dmstor.c a once over?

Thanks for this suggestion - I will probably look into it in connection with my work on issue #597 (if commenters agree #597 is a good idea): Kristian introduced proj_-namespace versions of dmstor and rtodms a few months ago, so I'll be handling those anyway.

Member

busstoptaktik commented Oct 10, 2017

Given that you're improving proj_stdtod.c, how about giving dmstor.c a once over?

Thanks for this suggestion - I will probably look into it in connection with my work on issue #597 (if commenters agree #597 is a good idea): Kristian introduced proj_-namespace versions of dmstor and rtodms a few months ago, so I'll be handling those anyway.

@busstoptaktik busstoptaktik referenced this pull request Oct 10, 2017

Open

Improve dmstor.c #598

@cffk

This comment has been minimized.

Show comment
Hide comment
@cffk

cffk Oct 10, 2017

Contributor

I notice a few instances of trailing spaces in the new proj_strtod.c.
In order to avoid irrelevant "chatter" in git diff's it would be nice if
checked in source files went through some normalization. I suggest:

  • no tabs
  • no trailing spaces
  • no more than one consecutive blank line
  • file ends with a new line
  • file doesn't end with a blank line

The "no tabs" rule is not obeyed by the existing code base. Perhaps
this should be changed in one commit? (Also it's not always clear
whether they are 4-space or 8-space tabs.)

(A weaker "no tabs" rule would be to forbid tabs except at the
beginnings of lines.)

Contributor

cffk commented Oct 10, 2017

I notice a few instances of trailing spaces in the new proj_strtod.c.
In order to avoid irrelevant "chatter" in git diff's it would be nice if
checked in source files went through some normalization. I suggest:

  • no tabs
  • no trailing spaces
  • no more than one consecutive blank line
  • file ends with a new line
  • file doesn't end with a blank line

The "no tabs" rule is not obeyed by the existing code base. Perhaps
this should be changed in one commit? (Also it's not always clear
whether they are 4-space or 8-space tabs.)

(A weaker "no tabs" rule would be to forbid tabs except at the
beginnings of lines.)

@busstoptaktik

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Oct 10, 2017

Member

@cffk - I can wholeheartedly agree with most of your suggestions, but not with the "no consequtive blank lines" rule.

Splitting code into visually separate sections is very useful, and I would much prefer to have two or more blanks between functions (although the 12 blanks between proj_roundtrip() and proj_trans_obs() in pj_obs_api.c is definitely too much).

In header files, searching through families of related function prototypes is also much easier if the eye has something to aid it, and in cases where prototypes are prefixed by a one line descriptive comment, you would want blanks between each comment/prototype pair, and hence need more than one blank between different families of functions to discern.

Even within a function code block, I occasionally find it useful to use more than one blank (seldom, and in very special cases).

Even the most well thought out coding formatting standards must give way in cases where sticking to the standard means reduced communication efficiency: Source code is mostly human-to-human communication, and the communicative aspect is quite important.

Much of the PROJ.4 source was written in the days of 80x24 VDUs, where it made good communication sense to carefully cater for one's vertical headroom (hence the many cases of "assignments as function arguments" in the oldest code).

Today even I (who need high contrast and large letters to be able to see the source) have around 50 lines of code visible at once. And my younger and more clear-sighted colleagues tend to have much more than that.

So in today's coding environment, spending some vertical real estate in order to improve communications is often worthwhile.

But except for that, I agree. The annoying spurious blanks in proj_strtod.c was a result of my change of editor. I have corrected things now, and also updated my editor setup :-)

Member

busstoptaktik commented Oct 10, 2017

@cffk - I can wholeheartedly agree with most of your suggestions, but not with the "no consequtive blank lines" rule.

Splitting code into visually separate sections is very useful, and I would much prefer to have two or more blanks between functions (although the 12 blanks between proj_roundtrip() and proj_trans_obs() in pj_obs_api.c is definitely too much).

In header files, searching through families of related function prototypes is also much easier if the eye has something to aid it, and in cases where prototypes are prefixed by a one line descriptive comment, you would want blanks between each comment/prototype pair, and hence need more than one blank between different families of functions to discern.

Even within a function code block, I occasionally find it useful to use more than one blank (seldom, and in very special cases).

Even the most well thought out coding formatting standards must give way in cases where sticking to the standard means reduced communication efficiency: Source code is mostly human-to-human communication, and the communicative aspect is quite important.

Much of the PROJ.4 source was written in the days of 80x24 VDUs, where it made good communication sense to carefully cater for one's vertical headroom (hence the many cases of "assignments as function arguments" in the oldest code).

Today even I (who need high contrast and large letters to be able to see the source) have around 50 lines of code visible at once. And my younger and more clear-sighted colleagues tend to have much more than that.

So in today's coding environment, spending some vertical real estate in order to improve communications is often worthwhile.

But except for that, I agree. The annoying spurious blanks in proj_strtod.c was a result of my change of editor. I have corrected things now, and also updated my editor setup :-)

@busstoptaktik

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Oct 11, 2017

Member

@kbevers wrote:

As far as I understand the code in this PR it takes care of (...) most of the selftests in the PJ_*.c files. Actually it looks like you used that mechanism to create the builtins.gie file.

True: I modified the material in pj_generic_selftest.c and pj_run_selftest.c to do a formatted replication of the test material, rather than executing it.

(...some) tests that do not use pj_generic_selftest() (e.g. vgridshift and helmert) (...) fits in nicely in gie and the test data can be added manually,

I will try to do that. Likely in a new PR: I think it would be a good idea to get gie into the master branch, having gie builtins.gie added to the CI targets, then, in a series of new PRs, gradually removing what can safely be removed.

(The) API function tests (most notably in PJ_cart.c (...are) still not covered by this test setup if the selftest is removed from proj (...) Do you have a plan for that? Maybe we should use an existing test framework for that?

I'm not against using existing test frameworks, e.g. cmocka or cunit, but I'm not volunteering to implement it.

Rather, I'd probably extract the API tests in PJ_cart.c into a separate program, a companion to multistresstest.c, to form a suite of programs that both implement testing functionality and work as sample material/demo code. In fact, I think multistresstest.c is, in itself, an amazing demo of how to write multithreaded code in a platform independent manner (kudos to @warmerdam for that). If I could write an API test program, that was half as instructive as multistresstest.c, I would be very happy!

Do you plan on distributing gie alongside the rest of the package, or is it only meant as a development tool? I ask because I noted that gie.c is placed src/ and not in test/gie/.

Yes - I think gie, geodtest, nad2bin (and perhaps even multistresstest and the proposed APIstresstest) are the kind of "occasionally useful programs", that do no harm when they quietly sit around in /usr/local/bin, but may be highly useful once or twice a year.

Member

busstoptaktik commented Oct 11, 2017

@kbevers wrote:

As far as I understand the code in this PR it takes care of (...) most of the selftests in the PJ_*.c files. Actually it looks like you used that mechanism to create the builtins.gie file.

True: I modified the material in pj_generic_selftest.c and pj_run_selftest.c to do a formatted replication of the test material, rather than executing it.

(...some) tests that do not use pj_generic_selftest() (e.g. vgridshift and helmert) (...) fits in nicely in gie and the test data can be added manually,

I will try to do that. Likely in a new PR: I think it would be a good idea to get gie into the master branch, having gie builtins.gie added to the CI targets, then, in a series of new PRs, gradually removing what can safely be removed.

(The) API function tests (most notably in PJ_cart.c (...are) still not covered by this test setup if the selftest is removed from proj (...) Do you have a plan for that? Maybe we should use an existing test framework for that?

I'm not against using existing test frameworks, e.g. cmocka or cunit, but I'm not volunteering to implement it.

Rather, I'd probably extract the API tests in PJ_cart.c into a separate program, a companion to multistresstest.c, to form a suite of programs that both implement testing functionality and work as sample material/demo code. In fact, I think multistresstest.c is, in itself, an amazing demo of how to write multithreaded code in a platform independent manner (kudos to @warmerdam for that). If I could write an API test program, that was half as instructive as multistresstest.c, I would be very happy!

Do you plan on distributing gie alongside the rest of the package, or is it only meant as a development tool? I ask because I noted that gie.c is placed src/ and not in test/gie/.

Yes - I think gie, geodtest, nad2bin (and perhaps even multistresstest and the proposed APIstresstest) are the kind of "occasionally useful programs", that do no harm when they quietly sit around in /usr/local/bin, but may be highly useful once or twice a year.

@kbevers

This comment has been minimized.

Show comment
Hide comment
@kbevers

kbevers Oct 11, 2017

Member

I'm not against using existing test frameworks, e.g. cmocka or cunit, but I'm not volunteering to implement it.

Let's migrate the existing stuff to a simple program like the multistresstest for now. At some point I would like to go a step further and use an existing framework as I see some benefits to that.

One issue that I have with the current selftest is that it runs all the tests, no matter what. When enabling debug info the tests are very noisy and it is almost impossible to find the relevant sections of the output. That can be handled with gie by having many small input files, e.g. one for each projection. I think debugging would be a lot easier if each operation got it's own file instead of the massive "builtin.gie" that is currently used. That way I can just run gie merc.gie to inspect debug output for the merc tests.

Member

kbevers commented Oct 11, 2017

I'm not against using existing test frameworks, e.g. cmocka or cunit, but I'm not volunteering to implement it.

Let's migrate the existing stuff to a simple program like the multistresstest for now. At some point I would like to go a step further and use an existing framework as I see some benefits to that.

One issue that I have with the current selftest is that it runs all the tests, no matter what. When enabling debug info the tests are very noisy and it is almost impossible to find the relevant sections of the output. That can be handled with gie by having many small input files, e.g. one for each projection. I think debugging would be a lot easier if each operation got it's own file instead of the massive "builtin.gie" that is currently used. That way I can just run gie merc.gie to inspect debug output for the merc tests.

@kbevers

This comment has been minimized.

Show comment
Hide comment
@kbevers

kbevers Oct 11, 2017

Member

@cffk

I am generally for your suggestion. A pre-commit hook could be added to git that makes sure that trailing whitespace etc. is following whatever format we desire. Regarding tabs to spaces, I have made some clean ups in various files as I come along them. My experience is that expanding tabs to 4 spaces is generally good, but there are many situations where the resulting formatting is completely FUBAR. There must be tools that does reformatting of C code that could help in these situations, but as Thomas mentions, sometimes the rules must be broken to communicate the idea behind the code better.

Member

kbevers commented Oct 11, 2017

@cffk

I am generally for your suggestion. A pre-commit hook could be added to git that makes sure that trailing whitespace etc. is following whatever format we desire. Regarding tabs to spaces, I have made some clean ups in various files as I come along them. My experience is that expanding tabs to 4 spaces is generally good, but there are many situations where the resulting formatting is completely FUBAR. There must be tools that does reformatting of C code that could help in these situations, but as Thomas mentions, sometimes the rules must be broken to communicate the idea behind the code better.

@busstoptaktik

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Oct 11, 2017

Member

@kbevers wrote:

I think debugging would be a lot easier if each operation got it's own file instead of the massive "builtin.gie" that is currently used. That way I can just run gie merc.gie to inspect debug output for the merc tests.

I think this is a good idea, but it comes at the expense of having a gazillion of tiny files to handle. Another possibility would be to implement a "match" command line flag, in which case your example could be written as:

gie  -m merc  builtins.gie
Member

busstoptaktik commented Oct 11, 2017

@kbevers wrote:

I think debugging would be a lot easier if each operation got it's own file instead of the massive "builtin.gie" that is currently used. That way I can just run gie merc.gie to inspect debug output for the merc tests.

I think this is a good idea, but it comes at the expense of having a gazillion of tiny files to handle. Another possibility would be to implement a "match" command line flag, in which case your example could be written as:

gie  -m merc  builtins.gie
@kbevers

This comment has been minimized.

Show comment
Hide comment
@kbevers

kbevers Oct 11, 2017

Member

I think this is a good idea, but it comes at the expense of having a gazillion of tiny files to handle.

I regard this as a feature, not a bug :) As long as you can do gie * I don't think it is a problem.

Another possibility would be to implement a "match" command line flag, in which case your example could be written as:

This is likely to generate a lot of extra noise. Let's say there's a bug in the pipeline operation. It is likely to show up in a bunch of unrelated tests. Having a designated set of test transformations for the pipeline operation itself makes debugging simpler.

I don't expect to see this anytime soon, but a really cool thing to have would be a make test setup, that only runs the test that are affected by the code that has been changed since last make. I am not even sure this would be possibly to make, but it could speed up tests significantly (especially in CI).

Member

kbevers commented Oct 11, 2017

I think this is a good idea, but it comes at the expense of having a gazillion of tiny files to handle.

I regard this as a feature, not a bug :) As long as you can do gie * I don't think it is a problem.

Another possibility would be to implement a "match" command line flag, in which case your example could be written as:

This is likely to generate a lot of extra noise. Let's say there's a bug in the pipeline operation. It is likely to show up in a bunch of unrelated tests. Having a designated set of test transformations for the pipeline operation itself makes debugging simpler.

I don't expect to see this anytime soon, but a really cool thing to have would be a make test setup, that only runs the test that are affected by the code that has been changed since last make. I am not even sure this would be possibly to make, but it could speed up tests significantly (especially in CI).

@busstoptaktik

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Oct 11, 2017

Member

I regard this as a feature, not a bug :) As long as you can do gie * I don't think it is a problem.

The problem is really with the reporting part: gie is designed to report all errors as they are found, then do a statistical summary at the end of each file, and, if there is more than one input file, also a grand total of the statistics after the last file has been read.

So the reporting will have to be re-designed in cases where gie * or gie a lot of files will be typical use cases..

Member

busstoptaktik commented Oct 11, 2017

I regard this as a feature, not a bug :) As long as you can do gie * I don't think it is a problem.

The problem is really with the reporting part: gie is designed to report all errors as they are found, then do a statistical summary at the end of each file, and, if there is more than one input file, also a grand total of the statistics after the last file has been read.

So the reporting will have to be re-designed in cases where gie * or gie a lot of files will be typical use cases..

@busstoptaktik

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Oct 11, 2017

Member

This seems to run fine now, also during CI, so I intend to merge it thursday afternoon (UTC) .

Member

busstoptaktik commented Oct 11, 2017

This seems to run fine now, also during CI, so I intend to merge it thursday afternoon (UTC) .

busstoptaktik added some commits Oct 7, 2017

gie.c and builtins.gie now able to reproduce internal test results
improved docs, improved strtod - avoid precision loss for very long fractions

Switch gie.c to use same framework as cct.c

numerous improvements in proj_strtod.c and gie.c

Add gie to the build system
remove trailing whitespace
Add missing prototype for opt_strip_path()

A bunch of minor oops cleanups

Remove unused functiion cloumn()

Fighting the good fight trying to be *both* POSIX *and* Windows compatible

A few more improvements: 2 missing casts and a potentially uninitialized variable
Run 'gie builtins.gie' as part of Travis CI jobs
Touch up configuration files to support gie
@busstoptaktik

This comment has been minimized.

Show comment
Hide comment
@busstoptaktik

busstoptaktik Oct 12, 2017

Member

Everything's fine - it's just the OSX build that fails for reasons that has nothing to do with this PR. Merging immediately

Member

busstoptaktik commented Oct 12, 2017

Everything's fine - it's just the OSX build that fails for reasons that has nothing to do with this PR. Merging immediately

@busstoptaktik busstoptaktik merged commit 56157fb into OSGeo:master Oct 12, 2017

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
coverage/coveralls Coverage decreased (-1.02%) to 75.651%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@busstoptaktik busstoptaktik deleted the busstoptaktik:gie branch Oct 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment