kicad-developers team mailing list archive

[John Beard <john.j.beard@xxxxxxxxx>]

Hi Wayne,

On Sat, Oct 6, 2018 at 5:29 PM Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
> No problem.  I will test your patch once you post it.

Patch attached. Indeed, it was a missing include in the header that
probably worked until now due to serendipitous include ordering in the
existing CPPs.

This actually illustrates one nice thing about having unit tests CPPs
that include only a single KiCad header - they make sure that header
and any transitive includes are compilable even when the test CPP only
includes that one header (i.e. as recommended by the code guidelines:
6.2 Headers Without Unsatisfied Dependencies [1]).

Another way this can improved is when you have a .cpp/.h pair for a
self-contained class, the .cpp should include the corresponding .h
first before any other include. This ensures the header is compilable
by itself, and has no implicit inclusion criteria for use. This is
sometimes, but not always true in KiCad. (In this case, there is no
.cpp anyway, so the unit test was the first CPP to attempt to include
by itself).

> The problem is all of the inline functions defined in convert_to_biu.h.
> Until they are replaced with per application equivalents, we will
> continue to have to compile every common source file multiple times that
> use anything in this file.

Right, but in the case of dialog_page_settings.cpp in particular, the
only definition actually needed from here was IU_PER_MILS,
which can be equivalently retrieved, via the virtual BASE_SCREEN
interface, from the derived class, where IU_PER_MILS is properly set
for each compiled target (eeschema, pl_editor, pcbnew).

> This is odd as they are both in the COMMON_SRCS list.  Actually,
> color.cpp is included twice.  Once in KICAD_SRCS and once in COMMON_SRCS
> which includes KICAD_SRCS.  We should remove color.cpp from either
> KICAD_SRCS or COMMON_SRCS.  I see no need to build it twice.

Even more odd. Removing one of the colors.cpp from the
common/CMakeLists.cpp didn't stop qa_common having to specifiy it
again, though.

For colors.cpp specifically, this is an issue which will be resolved
when, as promised in colors.h, the removal of legacy support will make
the global colour table obsolete. So it's perhaps more of a curiosity
than an issue?

Regardless, if anyone can shed light on this, I'd appreciate it, as it
probably indicates something fishy somewhere?

Cheers,

John

[1]: http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html#header_depends

From d89cd6a36b9e595378123a4dd0177f7049b11201 Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@xxxxxxxxx>
Date: Fri, 5 Oct 2018 17:17:54 +0100
Subject: [PATCH] QA: Add TITLE_BLOCK tests

Add some unit tests on TITLE_BLOCK

This commit also requires some mocks so the libcommon stuff
can work:

* Needs a Kiface() function to be linkable
* Needs some stuff from common to be build specially
* Needs to define itself as one of the unit-having programs
  to appease the units defines.
---
 include/title_block.h          |  1 +
 qa/common/CMakeLists.txt       | 14 ++++++
 qa/common/common_mocks.cpp     | 29 +++++++++++
 qa/common/test_title_block.cpp | 89 ++++++++++++++++++++++++++++++++++
 4 files changed, 133 insertions(+)
 create mode 100644 qa/common/test_title_block.cpp

diff --git a/include/title_block.h b/include/title_block.h
index 4d651b64d..62192574d 100644
--- a/include/title_block.h
+++ b/include/title_block.h
@@ -25,6 +25,7 @@
 #define TITLE_BLOCK_H
 
 #include <wx/string.h>
+#include <wx/arrstr.h>
 #include <ki_exception.h>
 
 class OUTPUTFORMATTER;
diff --git a/qa/common/CMakeLists.txt b/qa/common/CMakeLists.txt
index e6c163296..f0b175295 100644
--- a/qa/common/CMakeLists.txt
+++ b/qa/common/CMakeLists.txt
@@ -32,7 +32,15 @@ add_executable( qa_common
     # The main test entry points
     test_module.cpp
 
+    # stuff from common due to...units?
+    ../../common/eda_text.cpp
+
+    # stuff from common which is needed...why?
+    ../../common/colors.cpp
+    ../../common/observable.cpp
+
     test_hotkey_store.cpp
+    test_title_block.cpp
     test_utf8.cpp
 
     geometry/test_fillet.cpp
@@ -43,6 +51,7 @@ include_directories(
     ${CMAKE_SOURCE_DIR}/include
     ${CMAKE_SOURCE_DIR}/polygon
     ${Boost_INCLUDE_DIR}
+    ${INC_AFTER}
 )
 
 target_link_libraries( qa_common
@@ -53,6 +62,11 @@ target_link_libraries( qa_common
     ${wxWidgets_LIBRARIES}
 )
 
+# we need to pretend to be something to appease the units code
+target_compile_definitions( qa_common
+    PRIVATE GERBVIEW
+)
+
 add_test( NAME common
     COMMAND qa_common
 )
\ No newline at end of file
diff --git a/qa/common/common_mocks.cpp b/qa/common/common_mocks.cpp
index 9b54ee6ec..1ac749693 100644
--- a/qa/common/common_mocks.cpp
+++ b/qa/common/common_mocks.cpp
@@ -27,6 +27,7 @@
  */
 
 #include <pgm_base.h>
+#include <kiface_i.h>
 
 
 struct PGM_TEST_FRAME : public PGM_BASE
@@ -39,4 +40,32 @@ PGM_BASE& Pgm()
 {
     static PGM_TEST_FRAME program;
     return program;
+}
+
+static struct IFACE : public KIFACE_I
+{
+    bool OnKifaceStart( PGM_BASE* aProgram, int aCtlBits ) override
+    {
+        return start_common( aCtlBits );
+    }
+
+    wxWindow* CreateWindow( wxWindow* aParent, int aClassId, KIWAY* aKiway, int aCtlBits = 0 ) override
+    {
+        return nullptr;
+    }
+
+    void* IfaceOrAddress( int aDataId ) override
+    {
+        return nullptr;
+    }
+
+    IFACE( const char* aDSOname, KIWAY::FACE_T aType ) :
+        KIFACE_I( aDSOname, aType )
+    {}
+
+} kiface( "common_test", KIWAY::KIWAY_FACE_COUNT );
+
+KIFACE_I& Kiface()
+{
+    return kiface;
 }
\ No newline at end of file
diff --git a/qa/common/test_title_block.cpp b/qa/common/test_title_block.cpp
new file mode 100644
index 000000000..112194524
--- /dev/null
+++ b/qa/common/test_title_block.cpp
@@ -0,0 +1,89 @@
+/*
+ * 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
+ */
+
+#include <boost/test/unit_test.hpp>
+#include <boost/test/test_case_template.hpp>
+
+#include <title_block.h>
+
+
+struct TitleBlockFixture
+{
+    TitleBlockFixture()
+    {
+        m_tb.SetTitle( "title" );
+        m_tb.SetDate( "date" );
+        m_tb.SetCompany( "company" );
+
+        // leave revision blank
+        //m_tb.SetRevision( "revision" );
+
+        // set more than one comment to make sure the indexing of comments works
+        m_tb.SetComment1( "comment1" );
+        m_tb.SetComment2( "comment2" );
+        m_tb.SetComment3( "comment3" );
+        m_tb.SetComment4( "comment4" );
+    }
+
+    TITLE_BLOCK m_tb;
+};
+
+
+/**
+ * Declares a struct as the Boost test fixture.
+ */
+BOOST_FIXTURE_TEST_SUITE( TitleBlock, TitleBlockFixture )
+
+/**
+ * Check basic setting and getting of values
+ */
+BOOST_AUTO_TEST_CASE( SimpleAccess )
+{
+    BOOST_CHECK_EQUAL( "title", m_tb.GetTitle() );
+    BOOST_CHECK_EQUAL( "date", m_tb.GetDate() );
+    BOOST_CHECK_EQUAL( "company", m_tb.GetCompany() );
+
+    // This one is blank
+    BOOST_CHECK_EQUAL( "", m_tb.GetRevision() );
+
+    BOOST_CHECK_EQUAL( "comment1", m_tb.GetComment1() );
+    BOOST_CHECK_EQUAL( "comment2", m_tb.GetComment2() );
+    BOOST_CHECK_EQUAL( "comment3", m_tb.GetComment3() );
+    BOOST_CHECK_EQUAL( "comment4", m_tb.GetComment4() );
+}
+
+/*
+ * Check copy construction
+ */
+BOOST_AUTO_TEST_CASE( Copy )
+{
+    TITLE_BLOCK tb_cpy = m_tb;
+
+    // Check that values came through
+    BOOST_CHECK_EQUAL( "title", tb_cpy.GetTitle() );
+    BOOST_CHECK_EQUAL( "comment1", tb_cpy.GetComment1() );
+    BOOST_CHECK_EQUAL( "comment2", tb_cpy.GetComment2() );
+}
+
+
+BOOST_AUTO_TEST_SUITE_END()
-- 
2.19.0