From 4bd661add74ab9b58041663d93e7d1c58e424d69 Mon Sep 17 00:00:00 2001 From: John Beard Date: Mon, 15 Apr 2019 18:29:03 +0100 Subject: [PATCH] QA: Add test on GetLayerName function This adds a test to the recent GetLayerName function, and moves it to the base.h file for re-use for any code that needs a layer name. This test covers the case that caused lp:1824750 [1]. [1]: https://bugs.launchpad.net/kicad/+bug/1824750 --- qa/utils/kicad2step/pcb/test_base.cpp | 79 +++++++++++++++++++++++++-- utils/kicad2step/CMakeLists.txt | 5 +- utils/kicad2step/pcb/base.cpp | 24 ++++++++ utils/kicad2step/pcb/base.h | 13 +++++ utils/kicad2step/pcb/kicadcurve.cpp | 36 +----------- 5 files changed, 112 insertions(+), 45 deletions(-) diff --git a/qa/utils/kicad2step/pcb/test_base.cpp b/qa/utils/kicad2step/pcb/test_base.cpp index 05fee41f5e..f8e35d20b1 100644 --- a/qa/utils/kicad2step/pcb/test_base.cpp +++ b/qa/utils/kicad2step/pcb/test_base.cpp @@ -59,11 +59,17 @@ constexpr double DegToRad( double aDeg ) } +class TEST_PCB_BASE_FIXTURE +{ +public: + SEXPR::PARSER m_parser; +}; + + /** * Declare the test suite */ -BOOST_AUTO_TEST_SUITE( PcbBase ) - +BOOST_FIXTURE_TEST_SUITE( PcbBase, TEST_PCB_BASE_FIXTURE ) struct TEST_2D_POS_ROT { @@ -98,15 +104,13 @@ BOOST_AUTO_TEST_CASE( SexprTo2DPosAndRot ) {}, }, { - "(att 3.14 4.12 90.4)", + "(att 3.14 4.12 90.4)", // bad symbol name false, {}, {}, }, }; - SEXPR::PARSER parser; - for( const auto& c : cases ) { BOOST_TEST_CONTEXT( c.m_sexp ) @@ -114,7 +118,7 @@ BOOST_AUTO_TEST_CASE( SexprTo2DPosAndRot ) DOUBLET gotPos; double gotRot; - std::unique_ptr sexpr( parser.Parse( c.m_sexp ) ); + std::unique_ptr sexpr( m_parser.Parse( c.m_sexp ) ); const bool ret = Get2DPositionAndRotation( sexpr.get(), gotPos, gotRot ); @@ -132,4 +136,67 @@ BOOST_AUTO_TEST_CASE( SexprTo2DPosAndRot ) } } +struct TEST_LAYER_NAME +{ + std::string m_sexp; + bool m_valid; + std::string m_layer; +}; + +/** + * Test the layer list parser. + */ +BOOST_AUTO_TEST_CASE( TestGetLayerName ) +{ + const std::vector cases = { + { + // Quoted string - OK + "(layer \"Edge.Cuts\")", + true, + "Edge.Cuts", + }, + { + // Old KiCads exported without quotes (so, as symbols) + "(layer Edge.Cuts)", + true, + "Edge.Cuts", + }, + { + // No atoms + "(layer)", + false, + {}, + }, + { + // Too many atoms in list + "(layer \"Edge.Cuts\" 2)", + false, + {}, + }, + { + // Wrong atom type + "(layer 2)", + false, + {}, + }, + }; + + for( const auto& c : cases ) + { + BOOST_TEST_CONTEXT( c.m_sexp ) + { + std::unique_ptr sexpr( m_parser.Parse( c.m_sexp ) ); + + const OPT ret = GetLayerName( *sexpr ); + + BOOST_CHECK_EQUAL( ret.has_value(), c.m_valid ); + + if( ret ) + { + BOOST_CHECK_EQUAL( *ret, c.m_layer ); + } + } + } +} + BOOST_AUTO_TEST_SUITE_END() \ No newline at end of file diff --git a/utils/kicad2step/CMakeLists.txt b/utils/kicad2step/CMakeLists.txt index 9d74a77309..4589cc0cee 100644 --- a/utils/kicad2step/CMakeLists.txt +++ b/utils/kicad2step/CMakeLists.txt @@ -1,7 +1,3 @@ -include_directories( BEFORE - ${CMAKE_SOURCE_DIR}/include -) - include_directories( SYSTEM ${OCE_INCLUDE_DIRS} ${OCC_INCLUDE_DIR} @@ -26,6 +22,7 @@ add_library( kicad2step_lib STATIC target_include_directories( kicad2step_lib PUBLIC ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_SOURCE_DIR}/include # for core ) target_link_libraries( kicad2step_lib diff --git a/utils/kicad2step/pcb/base.cpp b/utils/kicad2step/pcb/base.cpp index a173a3d604..121f750ab3 100644 --- a/utils/kicad2step/pcb/base.cpp +++ b/utils/kicad2step/pcb/base.cpp @@ -257,3 +257,27 @@ bool GetXYZRotation( SEXPR::SEXPR* data, TRIPLET& aRotation ) return true; } + + +OPT GetLayerName( const SEXPR::SEXPR& aLayerElem ) +{ + OPT layer; + + if( aLayerElem.GetNumberOfChildren() == 2 ) + { + const auto& layerChild = *aLayerElem.GetChild( 1 ); + + // The layer child can be quoted (string) or unquoted (symbol) + // depending on PCB version. + if( layerChild.IsString() ) + { + layer = layerChild.GetString(); + } + else if( layerChild.IsSymbol() ) + { + layer = layerChild.GetSymbol(); + } + } + + return layer; +} \ No newline at end of file diff --git a/utils/kicad2step/pcb/base.h b/utils/kicad2step/pcb/base.h index 4ea21aadeb..f1c78581d4 100644 --- a/utils/kicad2step/pcb/base.h +++ b/utils/kicad2step/pcb/base.h @@ -30,6 +30,8 @@ #ifndef KICADBASE_H #define KICADBASE_H +#include + #include ///> Minimum distance between points to treat them as separate ones (mm) @@ -95,4 +97,15 @@ bool Get2DCoordinate( SEXPR::SEXPR* data, DOUBLET& aCoordinate ); bool Get3DCoordinate( SEXPR::SEXPR* data, TRIPLET& aCoordinate ); bool GetXYZRotation( SEXPR::SEXPR* data, TRIPLET& aRotation ); +/** + * Get the layer name from a layer element, if the layer is syntactically + * valid. + * + * E.g. (layer "Edge.Cuts") -> "Edge.Cuts" + * + * @param aLayerElem the s-expr element to get the name from + * @return the layer name if valid, else empty + */ +OPT GetLayerName( const SEXPR::SEXPR& aLayerElem ); + #endif // KICADBASE_H diff --git a/utils/kicad2step/pcb/kicadcurve.cpp b/utils/kicad2step/pcb/kicadcurve.cpp index 2b8818e34c..debbea3ced 100644 --- a/utils/kicad2step/pcb/kicadcurve.cpp +++ b/utils/kicad2step/pcb/kicadcurve.cpp @@ -25,46 +25,12 @@ #include -#include - #include #include #include #include -/** - * Get the layer name from a layer element, if the layer is syntactically - * valid - * - * E.g. (layer "Edge.Cuts") -> "Edge.Cuts" - * - * @param aLayerElem the s-expr element to get the name from - * @return the layer name if valid, else empty - */ -static OPT getLayerName( const SEXPR::SEXPR& aLayerElem ) -{ - OPT layer; - - if( aLayerElem.GetNumberOfChildren() == 2 ) - { - const auto& layerChild = *aLayerElem.GetChild( 1 ); - - // The layer child can be quoted (string) or unquoted (symbol) - // depending on PCB version. - if( layerChild.IsString() ) - { - layer = layerChild.GetString(); - } - else if( layerChild.IsSymbol() ) - { - layer = layerChild.GetSymbol(); - } - } - - return layer; -} - KICADCURVE::KICADCURVE() { @@ -152,7 +118,7 @@ bool KICADCURVE::Read( SEXPR::SEXPR* aEntry, CURVE_TYPE aCurveType ) } else if( text == "layer" ) { - const OPT layer = getLayerName( *child ); + const OPT layer = GetLayerName( *child ); if( !layer ) {