kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

I'm not sure what bothers me more, the fact the test fails or the fact
that the test used  different fillet code as the pass/fail criteria
rather than a hard coded known correct test.  Which fillet code was/is
correct?  We really do need to start doing a better job with our
geometry tests and testing in general.  Is the fillet code we use to
generate polygons and other geometry actually correct?

As far as fixing qa tests, I don't think that will impact the v5 release
but pushing it off to 5.0.1 or 5.1 probably makes more sense at this
point.  I do agree that there should be an easier way to run tests.

Cheers,

Wayne

On 7/9/2018 5:35 AM, John Beard wrote:
> Note to Wayne: Nothing here concerns v5 release, I was just trying to
> get a geom test working for future.
> 
> On Mon, Jul 9, 2018 at 9:12 AM, John Beard <john.j.beard@xxxxxxxxx> wrote:
>> Hi,
>>
>> Let's come back to the make test behaviour after v5, I think we'll
>> need to discuss that separately. However, I think this does illustrate
>> why we need the tests to be runnable easily, otherwise they suffer
>> bit-rot, and then the tests are useless.
>>
>> Looking at that change, the test is now iterating the second parameter
>> which is now "max error", not "number of segments". Does this test
>> still make any sense? It's now comparing:
>>
>>     SHAPE_POLY_SET::Fillet( radius, error );
>>
>> with
>>
>>     CPolyLine::Fillet( radius, segments );
>>
>> The original test was designed to ensure SHAPE_POLY_SET::Fillet and
>> CPolyLine::Fillet were the same, but they now have different
>> interfaces and semantics. Wouldn't it be better to check
>> SHAPE_POLY_SET::Fillet (and Chamfer) against some ground truth?
>>
>> Cheers,
>>
>> John
>>
>> On Fri, Jul 6, 2018 at 8:40 PM, Nick Østergaard <oe.nick@xxxxxxxxx> wrote:
>>> Hi
>>>
>>> I guess we could add it to the qa target somehow? What I don't particularyly
>>> like with this patch is that executing "make test" does not check for
>>> dependency changes.
>>>
>>> Back to the status about qa_geometry... it did pass a long time ago, doing a
>>> bit of git bisect points at this commit as the one breaking the test.
>>>
>>> fbf10e941bdf26bb3618aba0a1b7c44fd0bafed2 is the first bad commit
>>> commit fbf10e941bdf26bb3618aba0a1b7c44fd0bafed2
>>> Author: Jeff Young
>>> Date:   Thu Mar 22 18:02:45 2018 +0000
>>>
>>>     Switch zone fillets to absolute-error algorithm.
>>>
>>>     And some general cleanup to related constants, etc.
>>>
>>> :040000 040000 8b6ad8d44a7b38e0355ce5c8897f823d6255f811
>>> 8d54d43a9bd6e5062d6b804890a779e785e430cc M    common
>>> :040000 040000 5a90dc20fe7cb3f74ae1768a5b5024a902c9354d
>>> a2be92ebd64fd46ad17427e8e3c12da7f10df699 M    include
>>> :040000 040000 af9f333c0f56dca3a90fb7b04f385dbf39425e8d
>>> 99b5f9757c78216a08220b7eb056f343658b961d M    pcbnew
>>>
>>>
>>> Den tor. 5. jul. 2018 kl. 12.13 skrev John Beard <john.j.beard@xxxxxxxxx>:
>>>>
>>>> Hi,
>>>>
>>>> Are the qa_geometry test supposed to all work?
>>>>
>>>> When I run `qa_geometry`, I get 1160 errors like this:
>>>>
>>>> error: in "ChamferFillet/Fillet": check { chainPoints.begin(),
>>>> chainPoints.end() } == { polyPoints.begin(), polyPoints.end() } has
>>>> failed.
>>>>
>>>> Mismatch at position 0: [ 40 | 14 ] != [ 40 | 12 ]
>>>> Mismatch at position 1: [ 40 | 15 ] != [ 40 | 13 ]
>>>> Mismatch at position 2: [ 44 | 10 ] != [ 40 | 14 ]
>>>> Mismatch at position 3: [ 44 | 18 ] != [ 40 | 15 ]
>>>> Mismatch at position 4: [ 50 | 10 ] != [ 40 | 16 ]
>>>> Mismatch at position 5: [ 51 | 14 ] != [ 40 | 17 ]
>>>> Collections size mismatch: 6 != 25
>>>>
>>>> Attached is a patch that enabled CTest tests and adds qa_geometry as a
>>>> test. Then you can run `make test` or `ctest` to run all tests. I
>>>> think it would be good to have a single unambigous and easily
>>>> understood command to be able to run unit tests?
>>>>
>>>> This patch explicitly excludes the "ChamferFillet/Fillet" tests as
>>>> they are failing, but if those tests can be fixed, it would be good to
>>>> run them too.
>>>>
>>>> Cheers,
>>>>
>>>> John
>>>> _______________________________________________
>>>> Mailing list: https://launchpad.net/~kicad-developers
>>>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>>> More help   : https://help.launchpad.net/ListHelp
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>