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 <[email protected]> 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 <[email protected]> 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 <[email protected]>: >>>> >>>> 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 : [email protected] >>>> Unsubscribe : https://launchpad.net/~kicad-developers >>>> More help : https://help.launchpad.net/ListHelp > > _______________________________________________ > Mailing list: https://launchpad.net/~kicad-developers > Post to : [email protected] > Unsubscribe : https://launchpad.net/~kicad-developers > More help : https://help.launchpad.net/ListHelp > _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
