From c37b16eb0c8aeea3ca01d97f53aa85c3a559f715 Mon Sep 17 00:00:00 2001 From: Amos Jeffries Date: Mon, 29 Jun 2020 15:50:56 +0000 Subject: [PATCH] Cleanup unit tests (#688) * Prune dependencies for testUrl * Prune dependencies for testEventLoop * Remove obsolete stub_SwapDir.cc * Refactor testEventLoop to remove bitrot and obsolete Dispatcher code --- src/CompletionDispatcher.cc | 11 -- src/CompletionDispatcher.h | 31 ----- src/Makefile.am | 235 +----------------------------------- src/tests/Stub.am | 1 - src/tests/stub_SwapDir.cc | 48 -------- src/tests/stub_time.cc | 2 +- src/tests/testEventLoop.cc | 194 ++++++++--------------------- src/tests/testEventLoop.h | 21 +--- 8 files changed, 56 insertions(+), 487 deletions(-) delete mode 100644 src/CompletionDispatcher.cc delete mode 100644 src/CompletionDispatcher.h delete mode 100644 src/tests/stub_SwapDir.cc diff --git a/src/CompletionDispatcher.cc b/src/CompletionDispatcher.cc deleted file mode 100644 index 9eddde251e..0000000000 --- a/src/CompletionDispatcher.cc +++ /dev/null @@ -1,11 +0,0 @@ -/* - * Copyright (C) 1996-2020 The Squid Software Foundation and contributors - * - * Squid software is distributed under GPLv2+ license and includes - * contributions from numerous individuals and organizations. - * Please see the COPYING and CONTRIBUTORS files for details. - */ - -#include "squid.h" -#include "CompletionDispatcher.h" - diff --git a/src/CompletionDispatcher.h b/src/CompletionDispatcher.h deleted file mode 100644 index 20435b943d..0000000000 --- a/src/CompletionDispatcher.h +++ /dev/null @@ -1,31 +0,0 @@ -/* - * Copyright (C) 1996-2020 The Squid Software Foundation and contributors - * - * Squid software is distributed under GPLv2+ license and includes - * contributions from numerous individuals and organizations. - * Please see the COPYING and CONTRIBUTORS files for details. - */ - -#ifndef SQUID_COMPLETIONDISPATCHER_H -#define SQUID_COMPLETIONDISPATCHER_H - -/* Dispatch code to handle events that have completed. Completed events are queued - * with a completion dispatcher by the OS Async engine - i.e. the poll or kqueue or - * select loop, or a signal receiver, or the diskd/diskthreads/etc modules. - */ - -class CompletionDispatcher -{ - -public: - - virtual ~CompletionDispatcher() {} - - /* dispatch events. This should return true if there were events dispatched - * between the last call to dispatch() returning and this call returning. - */ - virtual bool dispatch() = 0; -}; - -#endif /* SQUID_COMPLETIONDISPATCHER_H */ - diff --git a/src/Makefile.am b/src/Makefile.am index 72e5611d7a..6452db0abd 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -241,8 +241,6 @@ squid_SOURCES = \ CommRead.h \ CommandLine.cc \ CommandLine.h \ - CompletionDispatcher.cc \ - CompletionDispatcher.h \ ConfigOption.cc \ ConfigParser.cc \ ConfigParser.h \ @@ -1002,7 +1000,6 @@ nodist_tests_testURL_SOURCES = \ tests/stub_libmem.cc tests_testURL_LDADD = \ libsquid.la \ - proxyp/libproxyp.la \ anyp/libanyp.la \ parser/libparser.la \ base/libbase.la \ @@ -3097,237 +3094,15 @@ tests_testEvent_LDFLAGS = $(LIBADD_DL) check_PROGRAMS += tests/testEventLoop tests_testEventLoop_SOURCES = \ - $(DELAY_POOL_SOURCE) \ - $(DNSSOURCE) \ - $(HTCPSOURCE) \ - $(IPC_SOURCE) \ - $(SNMP_SOURCE) \ - $(UNLINKDSOURCE) \ - $(WIN32_SOURCE) \ - AccessLogEntry.cc \ - AuthReg.h \ - BodyPipe.cc \ - tests/stub_CacheDigest.cc \ - CacheDigest.h \ - CachePeer.cc \ - CachePeer.h \ - ClientInfo.h \ - tests/stub_CollapsedForwarding.cc \ - ConfigOption.cc \ - ConfigParser.cc \ - CpuAffinityMap.cc \ - CpuAffinityMap.h \ - CpuAffinitySet.cc \ - CpuAffinitySet.h \ - tests/stub_ETag.cc \ - EventLoop.cc \ tests/testEventLoop.cc \ - EventLoop.h \ - tests/testEventLoop.h \ - ExternalACLEntry.cc \ - FadingCounter.cc \ - FileMap.h \ - FwdState.cc \ - FwdState.h \ - HappyConnOpener.cc \ - HappyConnOpener.h \ - HttpBody.cc \ - HttpBody.h \ - tests/stub_HttpControlMsg.cc \ - HttpHdrCc.cc \ - HttpHdrCc.h \ - HttpHdrContRange.cc \ - HttpHdrRange.cc \ - HttpHdrSc.cc \ - HttpHdrScTarget.cc \ - HttpHeader.cc \ - HttpHeader.h \ - HttpHeaderFieldInfo.h \ - HttpHeaderFieldStat.h \ - HttpHeaderTools.cc \ - HttpHeaderTools.h \ - HttpReply.cc \ - HttpRequest.cc \ - tests/stub_HttpUpgradeProtocolAccess.cc \ - IoStats.h \ - LogTags.cc \ - MasterXaction.cc \ - MasterXaction.h \ - MemBuf.cc \ - MemObject.cc \ - MemStore.cc \ - Notes.cc \ - Notes.h \ - Parsing.cc \ - PeerPoolMgr.cc \ - PeerPoolMgr.h \ - Pipeline.cc \ - Pipeline.h \ - RefreshPattern.h \ - RemovalPolicy.cc \ - RequestFlags.cc \ - RequestFlags.h \ - ResolvedPeers.cc \ - ResolvedPeers.h \ - tests/stub_SBufDetailedStats.cc \ - SquidMath.cc \ - SquidMath.h \ - StatCounters.cc \ - StatCounters.h \ - StatHist.cc \ - StatHist.h \ - StoreFileSystem.cc \ - StoreIOState.cc \ - tests/stub_StoreMeta.cc \ - StoreMetaUnpacker.cc \ - StoreSwapLogData.cc \ - StrList.cc \ - StrList.h \ - String.cc \ - Transients.cc \ - tests/stub_cache_cf.cc \ - cache_cf.h \ - cache_manager.cc \ - tests/stub_carp.cc \ - carp.h \ - cbdata.cc \ - clientStream.cc \ - tests/stub_client_db.cc \ - client_side.cc \ - client_side.h \ - client_side_reply.cc \ - client_side_request.cc \ - debug.cc \ - dlink.cc \ - dlink.h \ - errorpage.cc \ - event.cc \ - external_acl.cc \ - tests/stub_fatal.cc \ - fatal.h \ - fd.cc \ - fd.h \ - fde.cc \ - filemap.cc \ - fqdncache.cc \ - fqdncache.h \ - fs_io.cc \ - fs_io.h \ - tests/stub_gopher.cc \ - gopher.h \ - helper.cc \ - hier_code.h \ - http.cc \ - icp_v2.cc \ - icp_v3.cc \ - int.cc \ - int.h \ - internal.cc \ - internal.h \ - tests/stub_ipc_Forwarder.cc \ - ipcache.cc \ - tests/stub_libauth.cc \ - tests/stub_libauth_acls.cc \ - tests/stub_libdiskio.cc \ - tests/stub_libeui.cc \ - tests/stub_libmem.cc \ - tests/stub_libsecurity.cc \ - tests/stub_libstore.cc \ - tests/stub_main_cc.cc \ - mem_node.cc \ - mime.cc \ - mime.h \ - mime_header.cc \ - mime_header.h \ - multicast.cc \ - multicast.h \ - neighbors.cc \ - neighbors.h \ - pconn.cc \ - peer_digest.cc \ - peer_proxy_negotiate_auth.cc \ - peer_proxy_negotiate_auth.h \ - peer_select.cc \ - peer_sourcehash.cc \ - peer_sourcehash.h \ - peer_userhash.cc \ - peer_userhash.h \ - tests/stub_redirect.cc \ - redirect.h \ - refresh.cc \ - refresh.h \ - repl_modules.h \ - stat.cc \ - stat.h \ - stmem.cc \ - store.cc \ - store_client.cc \ - tests/stub_store_digest.cc \ - store_digest.h \ - store_io.cc \ - store_key_md5.cc \ - store_key_md5.h \ - store_log.cc \ - store_log.h \ - store_rebuild.cc \ - store_rebuild.h \ - tests/stub_store_stats.cc \ - store_swapin.cc \ - store_swapin.h \ - store_swapmeta.cc \ - store_swapout.cc \ - time.cc \ - tools.cc \ - tools.h \ - tests/stub_tunnel.cc \ - urn.cc \ - urn.h \ - tests/stub_wccp2.cc \ - wccp2.h \ - tests/stub_whois.cc \ - whois.h \ - wordlist.cc \ - wordlist.h + tests/testEventLoop.h nodist_tests_testEventLoop_SOURCES = \ - $(BUILT_SOURCES) + EventLoop.cc \ + tests/stub_debug.cc \ + tests/stub_fatal.cc \ + tests/stub_time.cc tests_testEventLoop_LDADD = \ - libsquid.la \ - clients/libclients.la \ - servers/libservers.la \ - ftp/libftp.la \ - helper/libhelper.la \ - http/libhttp.la \ - proxyp/libproxyp.la \ - parser/libparser.la \ - ident/libident.la \ - acl/libacls.la \ - acl/libstate.la \ - acl/libapi.la \ - dns/libdns.la \ base/libbase.la \ - ip/libip.la \ - fs/libfs.la \ - anyp/libanyp.la \ - icmp/libicmp.la \ - comm/libcomm.la \ - log/liblog.la \ - format/libformat.la \ - $(REPL_OBJS) \ - $(ADAPTATION_LIBS) \ - $(ESI_LIBS) \ - $(SSL_LIBS) \ - $(top_builddir)/lib/libmisccontainers.la \ - $(top_builddir)/lib/libmiscencoding.la \ - $(top_builddir)/lib/libmiscutil.la \ - ipc/libipc.la \ - mgr/libmgr.la \ - sbuf/libsbuf.la \ - store/libstore.la \ - $(SNMP_LIBS) \ - $(NETTLELIB) \ - $(REGEXLIB) \ - $(SSLLIB) \ - $(KRB5LIBS) \ $(LIBCPPUNIT_LIBS) \ $(COMPAT_LIB) \ $(XTRA_LIBS) diff --git a/src/tests/Stub.am b/src/tests/Stub.am index 19bf1246ae..9943f2236f 100644 --- a/src/tests/Stub.am +++ b/src/tests/Stub.am @@ -83,7 +83,6 @@ STUB_SOURCE= tests/STUB.h \ tests/stub_store_rebuild.cc \ tests/stub_store_stats.cc \ tests/stub_store_swapout.cc \ - tests/stub_SwapDir.cc \ tests/stub_time.cc \ tests/stub_tools.cc \ tests/stub_tunnel.cc \ diff --git a/src/tests/stub_SwapDir.cc b/src/tests/stub_SwapDir.cc deleted file mode 100644 index 10ebe1f0ff..0000000000 --- a/src/tests/stub_SwapDir.cc +++ /dev/null @@ -1,48 +0,0 @@ -/* - * Copyright (C) 1996-2020 The Squid Software Foundation and contributors - * - * Squid software is distributed under GPLv2+ license and includes - * contributions from numerous individuals and organizations. - * Please see the COPYING and CONTRIBUTORS files for details. - */ - -#include "squid.h" -#include "store/Disk.h" - -#define STUB_API "store/Disk.cc" -#include "tests/STUB.h" - -// SwapDir::SwapDir(char const *) STUB -// SwapDir::~SwapDir() STUB -void SwapDir::create() STUB -void SwapDir::dump(StoreEntry &) const STUB -bool SwapDir::doubleCheck(StoreEntry &) STUB_RETVAL(false) -void SwapDir::getStats(StoreInfoStats &) const STUB -void SwapDir::stat(StoreEntry &) const STUB -void SwapDir::statfs(StoreEntry &)const STUB -void SwapDir::maintain() STUB -uint64_t SwapDir::minSize() const STUB_RETVAL(0) -int64_t SwapDir::minObjectSize() const STUB_RETVAL(0) -int64_t SwapDir::maxObjectSize() const STUB_RETVAL(0) -void SwapDir::maxObjectSize(int64_t) STUB -void SwapDir::reference(StoreEntry &) STUB -bool SwapDir::dereference(StoreEntry &) STUB_RETVAL(false) -bool SwapDir::canStore(const StoreEntry &, int64_t, int &) const STUB_RETVAL(false) -bool SwapDir::canLog(StoreEntry const &)const STUB_RETVAL(false) -void SwapDir::openLog() STUB -void SwapDir::closeLog() STUB -int SwapDir::writeCleanStart() STUB_RETVAL(0) -void SwapDir::writeCleanDone() STUB -void SwapDir::logEntry(const StoreEntry &, int) const STUB -char const * SwapDir::type() const STUB_RETVAL("stub") -bool SwapDir::active() const STUB_RETVAL(false) -bool SwapDir::needsDiskStrand() const STUB_RETVAL(false) -ConfigOption * SwapDir::getOptionTree() const STUB_RETVAL(NULL) -void SwapDir::parseOptions(int) STUB -void SwapDir::dumpOptions(StoreEntry *) const STUB -bool SwapDir::optionReadOnlyParse(char const *, const char *, int) STUB_RETVAL(false) -void SwapDir::optionReadOnlyDump(StoreEntry *) const STUB -bool SwapDir::optionObjectSizeParse(char const *, const char *, int) STUB_RETVAL(false) -void SwapDir::optionObjectSizeDump(StoreEntry *) const STUB -StoreEntry * SwapDir::get(const cache_key *) STUB_RETVAL(NULL) - diff --git a/src/tests/stub_time.cc b/src/tests/stub_time.cc index 6c5d44cc0c..a5a8e6b466 100644 --- a/src/tests/stub_time.cc +++ b/src/tests/stub_time.cc @@ -25,5 +25,5 @@ const char * Time::FormatStrf(time_t ) STUB_RETVAL("") const char * Time::FormatHttpd(time_t ) STUB_RETVAL("") void TimeEngine::tick() STUB -TimeEngine::~TimeEngine() STUB +TimeEngine::~TimeEngine() {STUB_NOP} diff --git a/src/tests/testEventLoop.cc b/src/tests/testEventLoop.cc index 469f0a5bda..f61aefe93d 100644 --- a/src/tests/testEventLoop.cc +++ b/src/tests/testEventLoop.cc @@ -7,148 +7,75 @@ */ #include "squid.h" - -#include - #include "AsyncEngine.h" #include "EventLoop.h" -#include "mem/forward.h" #include "SquidTime.h" -#include "stat.h" -#include "testEventLoop.h" +#include "tests/testEventLoop.h" #include "unitTestMain.h" -CPPUNIT_TEST_SUITE_REGISTRATION( testEventLoop ); - -/* init legacy static-initialized modules */ +#include -void -testEventLoop::setUp() -{ - Mem::Init(); - statInit(); -} +CPPUNIT_TEST_SUITE_REGISTRATION( testEventLoop ); -/* - * Test creating a EventLoop - */ void testEventLoop::testCreate() { EventLoop(); } -#if POLISHED_MAIN_LOOP - -/* - * Running the loop once is useful for integration with other loops, such as - * migrating to it in incrementally. - * - * This test works by having a custom dispatcher and engine which record how - * many times they are called. - */ - -class RecordDispatcher : public CompletionDispatcher -{ - -public: - int calls; - RecordDispatcher(): calls(0) {} - - bool dispatch() { - ++calls; - /* claim we dispatched calls to be useful for the testStopOnIdle test. - */ - return true; - } -}; - -#endif /* POLISHED_MAIN_LOOP */ - class RecordingEngine : public AsyncEngine { - public: - int calls; - int lasttimeout; - int return_timeout; - RecordingEngine(int aTimeout=0): calls(0), lasttimeout(0), return_timeout(aTimeout) {} + RecordingEngine(int aTimeout = 0) : return_timeout(aTimeout) {} virtual int checkEvents(int timeout) { ++calls; lasttimeout = timeout; return return_timeout; } -}; -#if POLISHED_MAIN_LOOP + int calls = 0; + int lasttimeout = 0; + int return_timeout = 0; +}; +/* test that a registered async engine is invoked on each loop run + * we do this with an instrumented async engine. + */ void testEventLoop::testRunOnce() { - EventLoop theLoop; - RecordDispatcher dispatcher; - theLoop.registerDispatcher(&dispatcher); - RecordingEngine engine; - theLoop.registerEngine(&engine); - theLoop.runOnce(); - CPPUNIT_ASSERT_EQUAL(1, dispatcher.calls); - CPPUNIT_ASSERT_EQUAL(1, engine.calls); -} - -/* - * completion dispatchers registered with the event loop are invoked by the - * event loop. - * - * This test works by having a customer dispatcher which shuts the loop down - * once its been invoked twice. - * - * It also tests that loop.run() and loop.stop() work, because if they do not - * work, this test will either hang, or fail. - */ - -class ShutdownDispatcher : public CompletionDispatcher -{ - -public: - EventLoop &theLoop; - int calls; - ShutdownDispatcher(EventLoop & theLoop):theLoop(theLoop), calls(0) {} - - bool dispatch() { - if (++calls == 2) - theLoop.stop(); - - return true; + { + /* trivial case - no engine, should quit immediately */ + EventLoop theLoop; + CPPUNIT_ASSERT_EQUAL(true, theLoop.runOnce()); } -}; -void -testEventLoop::testRegisterDispatcher() -{ - EventLoop theLoop; - ShutdownDispatcher testDispatcher(theLoop); - theLoop.registerDispatcher(&testDispatcher); - theLoop.run(); - /* we should get two calls because the test dispatched returns true from - * dispatch(), and calls stop on the second call. - */ - CPPUNIT_ASSERT_EQUAL(2, testDispatcher.calls); -} + { + /* An event loop with all idle engines, and nothing dispatched in a run should + * automatically quit. The runOnce call should return True when the loop is + * entirely idle to make it easy for people running the loop by hand. + */ + EventLoop theLoop; + RecordingEngine engine(AsyncEngine::EVENT_IDLE); + theLoop.registerEngine(&engine); + CPPUNIT_ASSERT_EQUAL(true, theLoop.runOnce()); + CPPUNIT_ASSERT_EQUAL(1, engine.calls); + theLoop.run(); + CPPUNIT_ASSERT_EQUAL(2, engine.calls); + } -/* test that a registered async engine is invoked on each loop run - * we do this with an intstrumented async engine. - */ -void -testEventLoop::testRegisterEngine() -{ - EventLoop theLoop; - ShutdownDispatcher testDispatcher(theLoop); - theLoop.registerDispatcher(&testDispatcher); - RecordingEngine testEngine; - theLoop.registerEngine(&testEngine); - theLoop.run(); - CPPUNIT_ASSERT_EQUAL(2, testEngine.calls); + { + /* an engine that asks for a timeout should not be detected as idle: + * use runOnce which should return false + */ + EventLoop theLoop; + RecordingEngine engine; + theLoop.registerEngine(&engine); + CPPUNIT_ASSERT_EQUAL(false, theLoop.runOnce()); + CPPUNIT_ASSERT_EQUAL(1, engine.calls); + CPPUNIT_ASSERT_EQUAL(EVENT_LOOP_TIMEOUT, engine.lasttimeout); + } } /* each AsyncEngine needs to be given a timeout. We want one engine in each @@ -170,58 +97,31 @@ testEventLoop::testEngineTimeout() theLoop.registerEngine(&engineOne); theLoop.registerEngine(&engineTwo); theLoop.runOnce(); + CPPUNIT_ASSERT_EQUAL(1, engineOne.calls); CPPUNIT_ASSERT_EQUAL(0, engineOne.lasttimeout); + CPPUNIT_ASSERT_EQUAL(1, engineTwo.calls); CPPUNIT_ASSERT_EQUAL(5, engineTwo.lasttimeout); } -/* An event loop with all idle engines, and nothing dispatched in a run should - * automatically quit. The runOnce call should return True when the loop is - * entirely idle to make it easy for people running the loop by hand. +/* An engine which is suffering errors. This should result in 10 + * loops until the loop stops - because that's the error retry amount + * hard-coded into EventLoop::runOnce() */ void -testEventLoop::testStopOnIdle() +testEventLoop::testEngineErrors() { EventLoop theLoop; - /* trivial case - no dispatchers or engines, should quit immediately */ - CPPUNIT_ASSERT_EQUAL(true, theLoop.runOnce()); - theLoop.run(); - /* add a dispatcher with nothing to dispatch - use an EventDispatcher as its - * sufficient and handy - */ - EventDispatcher dispatcher; - theLoop.registerDispatcher(&dispatcher); - CPPUNIT_ASSERT_EQUAL(true, theLoop.runOnce()); - theLoop.run(); - /* add an engine which is idle. - */ - RecordingEngine engine(AsyncEngine::EVENT_IDLE); - theLoop.registerEngine(&engine); - CPPUNIT_ASSERT_EQUAL(true, theLoop.runOnce()); - CPPUNIT_ASSERT_EQUAL(1, engine.calls); - theLoop.run(); - CPPUNIT_ASSERT_EQUAL(2, engine.calls); - /* add an engine which is suffering errors. This should result in 10 - * loops until the loop stops - because that's the error retry amount - */ RecordingEngine failing_engine(AsyncEngine::EVENT_ERROR); theLoop.registerEngine(&failing_engine); CPPUNIT_ASSERT_EQUAL(false, theLoop.runOnce()); CPPUNIT_ASSERT_EQUAL(1, failing_engine.calls); + CPPUNIT_ASSERT_EQUAL(1, theLoop.errcount); theLoop.run(); /* run resets the error count ... */ + CPPUNIT_ASSERT_EQUAL(10, theLoop.errcount); CPPUNIT_ASSERT_EQUAL(11, failing_engine.calls); - - /* an engine that asks for a timeout should not be detected as idle: - * use runOnce which should return false - */ - theLoop = EventLoop(); - RecordingEngine non_idle_engine(1000); - theLoop.registerEngine(&non_idle_engine); - CPPUNIT_ASSERT_EQUAL(false, theLoop.runOnce()); } -#endif /* POLISHED_MAIN_LOOP */ - /* An event loop has a time service which is like an async engine but never * generates events and there can only be one such service. */ diff --git a/src/tests/testEventLoop.h b/src/tests/testEventLoop.h index e2c25fc2ad..40d17ab04d 100644 --- a/src/tests/testEventLoop.h +++ b/src/tests/testEventLoop.h @@ -19,33 +19,18 @@ class testEventLoop : public CPPUNIT_NS::TestFixture { CPPUNIT_TEST_SUITE( testEventLoop ); CPPUNIT_TEST( testCreate ); - -#if POLISHED_MAIN_LOOP CPPUNIT_TEST( testRunOnce ); - CPPUNIT_TEST( testRegisterDispatcher ); - CPPUNIT_TEST( testRegisterEngine ); CPPUNIT_TEST( testEngineTimeout ); - CPPUNIT_TEST( testStopOnIdle ); -#endif - + CPPUNIT_TEST( testEngineErrors ); CPPUNIT_TEST( testSetTimeService ); CPPUNIT_TEST( testSetPrimaryEngine ); CPPUNIT_TEST_SUITE_END(); -public: - void setUp(); - protected: void testCreate(); - -#if POLISHED_MAIN_LOOP - void testEngineTimeout(); void testRunOnce(); - void testRegisterDispatcher(); - void testRegisterEngine(); - void testStopOnIdle(); -#endif - + void testEngineTimeout(); + void testEngineErrors(); void testSetTimeService(); void testSetPrimaryEngine(); /* TODO: -- 2.47.2