kicad-developers team mailing list archive

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

Forwarding to the list (sorry!).

---------- Forwarded message ---------
From: John Beard <john.j.beard@xxxxxxxxx>
Date: Thu, Nov 8, 2018 at 12:09 PM
Subject: Re: [Kicad-developers] [PATCH]/Question TITLE_BLOCK tests
To: Wayne Stambaugh <stambaughw@xxxxxxxxx>


Hi Wayne,

Coming back to this now the fuzzing stuff seems to have settled in.

This is pretty much the same patch as before with only very minor
tweaks, but according to Jenkins, it builds on both Msys2 and MSVC. So
perhaps something has settled down after the recent merges?

As before, the tests introduced are trivial tests on TITLE_BLOCK - the
real aim is to get the libcommon mocks building, so new tests should
just be "drop-in" additions of .cpp files.

This patch depends on the legacy_gal patch posted yesterday, as
qa_common uses legacy_gal (which is why this is patch #0002).

Cheers,

John
On Mon, Oct 8, 2018 at 5:39 PM Wayne Stambaugh <stambaughw@xxxxxxxxx> wrote:
>
> Hi John,
>
> I am getting the following build error on windows with this patch:
>
> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe:
> ../../common/libgal.a(color4d.cpp.obj):color4d.cpp:(.text+0x1c9):
> undefined reference to `wxColourBase::FromString(wxString const&)'
> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe:
> ../../common/libgal.a(color4d.cpp.obj):color4d.cpp:(.text+0x32e):
> undefined reference to `wxColourBase::GetAsString(long) const'
> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe:
> ../../common/libgal.a(color4d.cpp.obj):color4d.cpp:(.text+0x80c):
> undefined reference to `wxColourBase::GetAsString(long) const'
> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe:
> ../../common/libgal.a(gal_display_options.cpp.obj):gal_display_options.cpp:(.text+0x22b):
> undefined reference to `wxConfigBase::Read(wxString const&, long*, long)
> const'
> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe:
> ../../common/libgal.a(gal_display_options.cpp.obj):gal_display_options.cpp:(.text+0x2b8):
> undefined reference to `wxConfigBase::Read(wxString const&, long*, long)
> const'
> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe:
> ../../common/libgal.a(gal_display_options.cpp.obj):gal_display_options.cpp:(.text+0x347):
> undefined reference to `wxConfigBase::Read(wxString const&, double*,
> double) const'
> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe:
> ../../common/libgal.a(gal_display_options.cpp.obj):gal_display_options.cpp:(.text+0x398):
> undefined reference to `wxConfigBase::Read(wxString const&, double*,
> double) const'
> collect2.exe: error: ld returned 1 exit status
> make[2]: *** [qa/common/CMakeFiles/qa_common.dir/build.make:217:
> qa/common/qa_common.exe] Error 1
> make[1]: *** [CMakeFiles/Makefile2:3111:
> qa/common/CMakeFiles/qa_common.dir/all]
>
>
>
> On 10/6/2018 3:48 PM, John Beard wrote:
> > 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 2ab589813df8c30cbd606751476e985392210613 Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@gmail.com>
Date: Fri, 5 Oct 2018 17:17:54 +0100
Subject: [PATCH 2/4] 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       | 16 ++++++
 qa/common/common_mocks.cpp     | 29 +++++++++++
 qa/common/test_title_block.cpp | 89 ++++++++++++++++++++++++++++++++++
 4 files changed, 135 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..ad17eeaab 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,16 +51,24 @@ include_directories(
     ${CMAKE_SOURCE_DIR}/include
     ${CMAKE_SOURCE_DIR}/polygon
     ${Boost_INCLUDE_DIR}
+    ${INC_AFTER}
 )
 
 target_link_libraries( qa_common
     common
+    legacy_gal
     polygon
     bitmaps
+    gal
     ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY}
     ${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.1