Hi, Some patches for LIB_TABLE_BASE:
1) Introduce some tests (which are good examples of when tests provide a clear usage example of the class, IMO). Expand some documentation and a little bit of style. 2) Change the GetCount to unsigned const-method, and the At method to take unsigned and return ref/const ref as needed. This makes it more STL-like (and consistent with its own internal implementation), and also means you can use it when the LIB_TABLE_BASE is const (this feeds into some work in progress, as well as being just a tidy-up). Cheers, John
From 7ff70ac8e43585c6522a958bd6588fda85f55229 Mon Sep 17 00:00:00 2001 From: John Beard <john.j.be...@gmail.com> Date: Sat, 2 Feb 2019 21:50:36 +0000 Subject: [PATCH 2/2] LIB_TABLE_BASE: Const and unsigned fixes * Make LIB_TABLE_BASE::GetCount() return unsigned. This is more consistent with the behaviour of STL containers (especially the boost::ptr_vector this is really accessing). Sadly wxGridTableBase() forces an int, so a cast is still required at the WX interface. * Make LIB_TABLE_BASE::At() return a reference. First, this is more consistent with normal STL indexing operator[]'s, and secondly, it allows an idiomatic const index method (so you can access const LIB_TABLE_ROWs from a const LIB_TABLE_BASE). The motivation is to allow use of this class and LIB_TABLE_ROW in a test program, where the LIB_TABLE_BASE is const. --- eeschema/dialogs/panel_sym_lib_table.cpp | 4 ++-- include/lib_table_base.h | 20 ++++++++++++++------ pcbnew/dialogs/panel_fp_lib_table.cpp | 4 ++-- qa/common/test_lib_table.cpp | 14 +++++++------- 4 files changed, 25 insertions(+), 17 deletions(-) diff --git a/eeschema/dialogs/panel_sym_lib_table.cpp b/eeschema/dialogs/panel_sym_lib_table.cpp index 659df04ba..5690a5a5b 100644 --- a/eeschema/dialogs/panel_sym_lib_table.cpp +++ b/eeschema/dialogs/panel_sym_lib_table.cpp @@ -122,11 +122,11 @@ protected: if( parsed ) { // make sure the table is big enough... - if( tmp_tbl.GetCount() > tbl->GetNumberRows() ) + if( tmp_tbl.GetCount() > (unsigned) tbl->GetNumberRows() ) tbl->AppendRows( tmp_tbl.GetCount() - tbl->GetNumberRows() ); for( int i = 0; i < tmp_tbl.GetCount(); ++i ) - tbl->rows.replace( i, tmp_tbl.At( i )->clone() ); + tbl->rows.replace( i, tmp_tbl.At( i ).clone() ); } m_grid->AutoSizeColumns( false ); diff --git a/include/lib_table_base.h b/include/lib_table_base.h index c60d6cec1..4f7227498 100644 --- a/include/lib_table_base.h +++ b/include/lib_table_base.h @@ -342,19 +342,27 @@ public: /** * Get the number of rows contained in the table */ - int GetCount() + unsigned GetCount() const { return rows.size(); } /** - * Get the row at the given index. - * @param aIndex row index (must exist) - * @return pointer to the row + * Get the 'n'th #LIB_TABLE_ROW object + * @param aIndex index of row (must exist: from 0 to GetCount() - 1) + * @return reference to the row */ - LIB_TABLE_ROW* At( int aIndex ) + LIB_TABLE_ROW& At( unsigned aIndex ) { - return &rows[aIndex]; + return rows[aIndex]; + } + + /** + * @copydoc At() + */ + const LIB_TABLE_ROW& At( unsigned aIndex ) const + { + return rows[aIndex]; } /** diff --git a/pcbnew/dialogs/panel_fp_lib_table.cpp b/pcbnew/dialogs/panel_fp_lib_table.cpp index e1ef84b1a..641b531ce 100644 --- a/pcbnew/dialogs/panel_fp_lib_table.cpp +++ b/pcbnew/dialogs/panel_fp_lib_table.cpp @@ -245,11 +245,11 @@ protected: if( parsed ) { // make sure the table is big enough... - if( tmp_tbl.GetCount() > tbl->GetNumberRows() ) + if( tmp_tbl.GetCount() > (unsigned) tbl->GetNumberRows() ) tbl->AppendRows( tmp_tbl.GetCount() - tbl->GetNumberRows() ); for( int i = 0; i < tmp_tbl.GetCount(); ++i ) - tbl->rows.replace( i, tmp_tbl.At( i )->clone() ); + tbl->rows.replace( i, tmp_tbl.At( i ).clone() ); } m_grid->AutoSizeColumns( false ); diff --git a/qa/common/test_lib_table.cpp b/qa/common/test_lib_table.cpp index 02088020f..380d0d5e9 100644 --- a/qa/common/test_lib_table.cpp +++ b/qa/common/test_lib_table.cpp @@ -261,7 +261,7 @@ BOOST_AUTO_TEST_CASE( Equal ) BOOST_CHECK_EQUAL( false, m_mainTableNoFb != m_mainTableWithFb ); // Modify one of them - m_mainTableWithFb.At( 1 )->SetNickName( "NewNickname" ); + m_mainTableWithFb.At( 1 ).SetNickName( "NewNickname" ); BOOST_CHECK_EQUAL( false, m_mainTableNoFb == m_mainTableWithFb ); BOOST_CHECK_EQUAL( true, m_mainTableNoFb != m_mainTableWithFb ); @@ -280,15 +280,15 @@ BOOST_AUTO_TEST_CASE( Indexing ) // Filled with the right row count BOOST_CHECK_EQUAL( m_mainTableNoFb.GetCount(), 3 ); - const auto* row0 = m_mainTableNoFb.At( 0 ); - BOOST_CHECK_EQUAL( row0->GetNickName(), "Lib1" ); + const auto& row0 = m_mainTableNoFb.At( 0 ); + BOOST_CHECK_EQUAL( row0.GetNickName(), "Lib1" ); - const auto* row1 = m_mainTableNoFb.At( 1 ); - BOOST_CHECK_EQUAL( row1->GetNickName(), "Lib2" ); + const auto& row1 = m_mainTableNoFb.At( 1 ); + BOOST_CHECK_EQUAL( row1.GetNickName(), "Lib2" ); // disable, but still in the index - const auto* row2 = m_mainTableNoFb.At( 2 ); - BOOST_CHECK_EQUAL( row2->GetNickName(), "Lib3" ); + const auto& row2 = m_mainTableNoFb.At( 2 ); + BOOST_CHECK_EQUAL( row2.GetNickName(), "Lib3" ); // check correct handling of out-of-bounds // TODO: this doesn't work with boost::ptr_vector - that only asserts -- 2.20.1
From a7c055881040ac16b3d37055b02ae2d6ce4d647a Mon Sep 17 00:00:00 2001 From: John Beard <john.j.be...@gmail.com> Date: Sun, 3 Feb 2019 11:44:52 +0000 Subject: [PATCH 1/2] QA: LIB_TABLE tests Some basic tests on LIB_TABLE and LIB_TABLE_ROW that demonstrate the behaviour of fallbacks and various access methods. Also add a few LIB_TABLE_BASE comments and changed some NULLs to nullptr. --- common/lib_table_base.cpp | 8 +- include/lib_table_base.h | 26 ++- qa/common/CMakeLists.txt | 1 + qa/common/test_lib_table.cpp | 364 +++++++++++++++++++++++++++++++++++ 4 files changed, 392 insertions(+), 7 deletions(-) create mode 100644 qa/common/test_lib_table.cpp diff --git a/common/lib_table_base.cpp b/common/lib_table_base.cpp index 5147d68d8..8c53009a4 100644 --- a/common/lib_table_base.cpp +++ b/common/lib_table_base.cpp @@ -306,7 +306,7 @@ LIB_TABLE_ROW* LIB_TABLE::findRow( const wxString& aNickName ) const // not found, search fall back table(s), if any } while( ( cur = cur->fallBack ) != 0 ); - return NULL; // not found + return nullptr; // not found } @@ -328,7 +328,7 @@ LIB_TABLE_ROW* LIB_TABLE::findRow( const wxString& aNickName ) // not found, search fall back table(s), if any } while( ( cur = cur->fallBack ) != 0 ); - return NULL; // not found + return nullptr; // not found } @@ -364,7 +364,7 @@ const LIB_TABLE_ROW* LIB_TABLE::FindRowByURI( const wxString& aURI ) // not found, search fall back table(s), if any } while( ( cur = cur->fallBack ) != 0 ); - return NULL; // not found + return nullptr; // not found } @@ -503,7 +503,7 @@ PROPERTIES* LIB_TABLE::ParseOptions( const std::string& aOptionsList ) return new PROPERTIES( props ); } - return NULL; + return nullptr; } diff --git a/include/lib_table_base.h b/include/lib_table_base.h index 6569956ac..c60d6cec1 100644 --- a/include/lib_table_base.h +++ b/include/lib_table_base.h @@ -304,7 +304,7 @@ public: * a row is not found in this table. No ownership is * taken of aFallBackTable. */ - LIB_TABLE( LIB_TABLE* aFallBackTable = NULL ); + LIB_TABLE( LIB_TABLE* aFallBackTable = nullptr ); virtual ~LIB_TABLE(); @@ -315,6 +315,12 @@ public: nickIndex.clear(); } + /** + * Compares this table against another. + * + * This compares the row *contents* against each other. + * Any fallback tables are not checked. + */ bool operator==( const LIB_TABLE& r ) const { if( rows.size() == r.rows.size() ) @@ -333,9 +339,23 @@ public: bool operator!=( const LIB_TABLE& r ) const { return !( *this == r ); } - int GetCount() { return rows.size(); } + /** + * Get the number of rows contained in the table + */ + int GetCount() + { + return rows.size(); + } - LIB_TABLE_ROW* At( int aIndex ) { return &rows[aIndex]; } + /** + * Get the row at the given index. + * @param aIndex row index (must exist) + * @return pointer to the row + */ + LIB_TABLE_ROW* At( int aIndex ) + { + return &rows[aIndex]; + } /** * Return true if the table is empty. diff --git a/qa/common/CMakeLists.txt b/qa/common/CMakeLists.txt index 9ac55d56b..229edab2c 100644 --- a/qa/common/CMakeLists.txt +++ b/qa/common/CMakeLists.txt @@ -41,6 +41,7 @@ set( common_srcs test_coroutine.cpp test_format_units.cpp test_hotkey_store.cpp + test_lib_table.cpp test_kicad_string.cpp test_refdes_utils.cpp test_title_block.cpp diff --git a/qa/common/test_lib_table.cpp b/qa/common/test_lib_table.cpp new file mode 100644 index 000000000..02088020f --- /dev/null +++ b/qa/common/test_lib_table.cpp @@ -0,0 +1,364 @@ +/* + * This program source code file is part of KiCad, a free EDA CAD application. + * + * Copyright (C) 2018 KiCad Developers, see CHANGELOG.TXT for contributors. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, you may find one here: + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.html + * or you may search the http://www.gnu.org website for the version 2 license, + * or you may write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA + */ + +/** + * @file + * Test suite for LIB_TABLE_BASE + * + * This test is of a abstract class, so we will implement a cut-down + * version and only test the core logic. Tests of the concrete implementations's + * own logic should be done in the relevant tests. + */ + +#include <unit_test_utils/unit_test_utils.h> + +// Code under test +#include <lib_table_base.h> + +#include <make_unique.h> + + +/** + * A concrete implementation of #LIB_TABLE_ROW that implements + * the minimum interface. + */ +class TEST_LIB_TABLE_ROW : public LIB_TABLE_ROW +{ +public: + TEST_LIB_TABLE_ROW( const wxString& aNick, const wxString& aURI, const wxString& aOptions, + const wxString& aDescr ) + : LIB_TABLE_ROW( aNick, aURI, aOptions, aDescr ) + { + } + + const wxString GetType() const override + { + return m_type; + } + + void SetType( const wxString& aType ) override + { + m_type = aType; + } + +private: + LIB_TABLE_ROW* do_clone() const override + { + return new TEST_LIB_TABLE_ROW( *this ); + } + + wxString m_type; +}; + + +/** + * A concrete implementation of #LIB_TABLE that implements + * the minimum interface for testing. + * + * Notably, the Parse/Format functions are not used, as there is no "real" + * format to read/write. + */ +class TEST_LIB_TABLE : public LIB_TABLE +{ +public: + TEST_LIB_TABLE( LIB_TABLE* aFallback = nullptr ) : LIB_TABLE( aFallback ) + { + } + + KICAD_T Type() override // from _ELEM + { + // Doesn't really matter what this is + return FP_LIB_TABLE_T; + } + +private: + void Parse( LIB_TABLE_LEXER* aLexer ) override + { + // Do nothing, we won't parse anything. Parse testing of actual data + // will happen in the relevant other tests. + } + + void Format( OUTPUTFORMATTER* aOutput, int aIndentLevel ) const override + { + // do nothing, we don't need to test this function + } +}; + + +/** + * Simple structure to contain data to set up a single #TEST_LIB_TABLE_ROW + */ +struct LIB_ROW_DEFINITION +{ + std::string m_nickname; + std::string m_uri; + std::string m_description; + bool m_enabled; +}; + + +// clang-format off +/** + * Set-up data for the re-used library row definitions. + */ +static const std::vector<LIB_ROW_DEFINITION> main_lib_defs = { + { + "Lib1", + "://lib/1", + "The first library", + true, + }, + { + "Lib2", + "://lib/2", + "The second library", + true, + }, + { + "Lib3", + "://lib/3", + "The third library", + false, + }, +}; + +static const std::vector<LIB_ROW_DEFINITION> fallback_lib_defs = { + { + "FallbackLib1", + "://lib/fb1", + "The first fallback library", + true, + }, + { + "FallbackLib2", + "://lib/fb2", + "The second fallback library", + false, + }, +}; +// clang-format on + + +/** + * Reusable test fixture with some basic pre-filled tables. + */ +struct LIB_TABLE_TEST_FIXTURE +{ + LIB_TABLE_TEST_FIXTURE() : m_mainTableWithFb( &m_fallbackTable ) + { + for( const auto& lib : main_lib_defs ) + { + m_mainTableNoFb.InsertRow( makeRowFromDef( lib ).release() ); + m_mainTableWithFb.InsertRow( makeRowFromDef( lib ).release() ); + } + + for( const auto& lib : fallback_lib_defs ) + { + m_fallbackTable.InsertRow( makeRowFromDef( lib ).release() ); + } + } + + /** + * Helper to construct a new #TEST_LIB_TABLE_ROW from a definition struct + */ + std::unique_ptr<TEST_LIB_TABLE_ROW> makeRowFromDef( const LIB_ROW_DEFINITION& aDef ) + { + auto row = std::make_unique<TEST_LIB_TABLE_ROW>( + aDef.m_nickname, aDef.m_uri, "", aDef.m_description ); + + row->SetEnabled( aDef.m_enabled ); + + return row; + } + + /// Table with some enabled and disabled libs, no fallback provided + TEST_LIB_TABLE m_mainTableNoFb; + + /// Identical to m_mainTableNoFb, but with a fallback + TEST_LIB_TABLE m_mainTableWithFb; + + /// The table that m_mainTableWithFb falls back to. + TEST_LIB_TABLE m_fallbackTable; +}; + +/** + * Declare the test suite + */ +BOOST_FIXTURE_TEST_SUITE( LibTable, LIB_TABLE_TEST_FIXTURE ) + +/** + * Check an empty table behaves correctly + */ +BOOST_AUTO_TEST_CASE( Empty ) +{ + TEST_LIB_TABLE table; + + // Tables start out empty + BOOST_CHECK_EQUAL( table.GetCount(), 0 ); + BOOST_CHECK_EQUAL( true, table.IsEmpty() ); +} + + +/** + * Check size and emptiness on tables with fallback + */ +BOOST_AUTO_TEST_CASE( EmptyWithFallback ) +{ + // Fall back though another empty table to the real fallback + TEST_LIB_TABLE interposer_table( &m_fallbackTable ); + TEST_LIB_TABLE table( &interposer_table ); + + // Table has no elements... + BOOST_CHECK_EQUAL( table.GetCount(), 0 ); + + // But it's not empty if we include the fallback + BOOST_CHECK_EQUAL( false, table.IsEmpty( true ) ); +} + + +/** + * Check table clearing function + */ +BOOST_AUTO_TEST_CASE( Clear ) +{ + m_mainTableNoFb.Clear(); + + // Tables start out empty + BOOST_CHECK_EQUAL( m_mainTableNoFb.GetCount(), 0 ); + BOOST_CHECK_EQUAL( true, m_mainTableNoFb.IsEmpty() ); +} + + +/** + * Check table equality function + */ +BOOST_AUTO_TEST_CASE( Equal ) +{ + // writing a boot print is a bit of faff, so just use BOOST_CHECK_EQUAL and bools + + // These two are identical, except the fallback (which isn't checked) + BOOST_CHECK_EQUAL( true, m_mainTableNoFb == m_mainTableWithFb ); + BOOST_CHECK_EQUAL( false, m_mainTableNoFb != m_mainTableWithFb ); + + // Modify one of them + m_mainTableWithFb.At( 1 )->SetNickName( "NewNickname" ); + BOOST_CHECK_EQUAL( false, m_mainTableNoFb == m_mainTableWithFb ); + BOOST_CHECK_EQUAL( true, m_mainTableNoFb != m_mainTableWithFb ); + + // And check unequal (against empty) + TEST_LIB_TABLE empty_table; + BOOST_CHECK_EQUAL( false, m_mainTableNoFb == empty_table ); + BOOST_CHECK_EQUAL( true, m_mainTableNoFb != empty_table ); +} + + +/** + * Test indexing into the main table + */ +BOOST_AUTO_TEST_CASE( Indexing ) +{ + // Filled with the right row count + BOOST_CHECK_EQUAL( m_mainTableNoFb.GetCount(), 3 ); + + const auto* row0 = m_mainTableNoFb.At( 0 ); + BOOST_CHECK_EQUAL( row0->GetNickName(), "Lib1" ); + + const auto* row1 = m_mainTableNoFb.At( 1 ); + BOOST_CHECK_EQUAL( row1->GetNickName(), "Lib2" ); + + // disable, but still in the index + const auto* row2 = m_mainTableNoFb.At( 2 ); + BOOST_CHECK_EQUAL( row2->GetNickName(), "Lib3" ); + + // check correct handling of out-of-bounds + // TODO: this doesn't work with boost::ptr_vector - that only asserts + // BOOST_CHECK_THROW( m_mainTableNoFb.At( 3 ), std::out_of_range ); +} + +/** + * Test retrieval of libs by nickname + */ +BOOST_AUTO_TEST_CASE( HasLibrary ) +{ + BOOST_CHECK_EQUAL( true, m_mainTableNoFb.HasLibrary( "Lib1" ) ); + + // disabled lib can be "not found" if checkEnabled is set + BOOST_CHECK_EQUAL( true, m_mainTableNoFb.HasLibrary( "Lib3" ) ); + BOOST_CHECK_EQUAL( false, m_mainTableNoFb.HasLibrary( "Lib3", true ) ); + + BOOST_CHECK_EQUAL( false, m_mainTableNoFb.HasLibrary( "NotPresent" ) ); +} + + +/** + * Test retrieval of libs by nickname + */ +BOOST_AUTO_TEST_CASE( Descriptions ) +{ + BOOST_CHECK_EQUAL( "The first library", m_mainTableNoFb.GetDescription( "Lib1" ) ); + + // disabled lib works + BOOST_CHECK_EQUAL( "The third library", m_mainTableNoFb.GetDescription( "Lib3" ) ); +} + + +/** + * Test retrieval of libs by URI + */ +BOOST_AUTO_TEST_CASE( URIs ) +{ + BOOST_CHECK_EQUAL( "://lib/1", m_mainTableNoFb.GetFullURI( "Lib1" ) ); + + const auto* row = m_mainTableNoFb.FindRowByURI( "://lib/1" ); + + // should be found + BOOST_CHECK_NE( nullptr, row ); + + if( row ) + { + BOOST_CHECK_EQUAL( "Lib1", row->GetNickName() ); + } + + row = m_mainTableNoFb.FindRowByURI( "this_uri_is_not_found" ); + + BOOST_CHECK_EQUAL( nullptr, row ); +} + +/** + * Test retrieval of the logical libs function + */ +BOOST_AUTO_TEST_CASE( LogicalLibs ) +{ + auto logical_libs = m_mainTableNoFb.GetLogicalLibs(); + + // The enabled library nicknames + const std::vector<wxString> exp_libs = { + "Lib1", + "Lib2", + }; + + BOOST_CHECK_EQUAL_COLLECTIONS( + logical_libs.begin(), logical_libs.end(), exp_libs.begin(), exp_libs.end() ); +} + +BOOST_AUTO_TEST_SUITE_END() \ No newline at end of file -- 2.20.1
_______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp