From a9bc740356f16b287540c8db910efba4c5068f39 Mon Sep 17 00:00:00 2001 From: Douglas Adler Date: Mon, 27 Jul 2026 09:14:57 -0500 Subject: [PATCH] Revert "Fixed Unit Tests to build reliable on docker environment (#568)" This reverts commit 0389de868489f31b723035f60a0167094357ca80. --- CMakeLists.txt | 4 +- ...ild_google_test.cmake => googletest.cmake} | 21 ++---- .../gstplayer/include/GstGenericPlayer.h | 11 +-- .../{interface => include}/IGstProfiler.h | 0 .../gstplayer/interface/IGstGenericPlayer.h | 27 +++---- .../gstplayer/source/GstGenericPlayer.cpp | 16 +--- scripts/gtest/build_and_run_tests.py | 73 +++---------------- .../gstplayer/genericPlayer/CreateTest.cpp | 9 ++- .../server/main/mediaPipeline/LoadTest.cpp | 4 +- .../base/MediaPipelineTestBase.cpp | 2 +- .../gstplayer/GstGenericPlayerFactoryMock.h | 3 +- 11 files changed, 44 insertions(+), 126 deletions(-) rename cmake/{build_google_test.cmake => googletest.cmake} (83%) rename media/server/gstplayer/{interface => include}/IGstProfiler.h (100%) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4eed45dd1..04c239071 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -245,7 +245,7 @@ if( CMAKE_BUILD_FLAG STREQUAL "UnitTests" ) # Turn off link time optimisation to speed up compilation set(CMAKE_INTERPROCEDURAL_OPTIMIZATION FALSE) - include( cmake/build_google_test.cmake ) + include( cmake/googletest.cmake ) add_subdirectory( tests/common EXCLUDE_FROM_ALL ) add_subdirectory( tests/unittests EXCLUDE_FROM_ALL ) @@ -258,7 +258,7 @@ if( CMAKE_BUILD_FLAG STREQUAL "ComponentTests" ) # Turn off link time optimisation to speed up compilation set(CMAKE_INTERPROCEDURAL_OPTIMIZATION FALSE) - include( cmake/build_google_test.cmake ) + include( cmake/googletest.cmake ) add_subdirectory( tests/common EXCLUDE_FROM_ALL ) add_subdirectory( tests/componenttests EXCLUDE_FROM_ALL ) diff --git a/cmake/build_google_test.cmake b/cmake/googletest.cmake similarity index 83% rename from cmake/build_google_test.cmake rename to cmake/googletest.cmake index 890c050f7..b6ece7eef 100644 --- a/cmake/build_google_test.cmake +++ b/cmake/googletest.cmake @@ -93,22 +93,11 @@ macro( add_gtests TESTNAME ) # gtest_discover_tests replaces gtest_add_tests, # see https://cmake.org/cmake/help/v3.10/module/GoogleTest.html for more options to pass to it - # DISCOVERY_MODE PRE_TEST defers test enumeration from cmake configure - # time to just before test execution. It was added in CMake 3.18; on older - # versions we omit it so configuration still works (falling back to the - # default POST_BUILD discovery mode). - if( CMAKE_VERSION VERSION_GREATER_EQUAL "3.18" ) - gtest_discover_tests( ${TESTNAME} - # set a working directory so your project root so that you can find - # test data via paths relative to the project root - WORKING_DIRECTORY ${PROJECT_DIR} - DISCOVERY_MODE PRE_TEST - ) - else() - gtest_discover_tests( ${TESTNAME} - WORKING_DIRECTORY ${PROJECT_DIR} - ) - endif() + gtest_discover_tests( ${TESTNAME} + # set a working directory so your project root so that you can find + # test data via paths relative to the project root + WORKING_DIRECTORY ${PROJECT_DIR} + ) set_target_properties( ${TESTNAME} PROPERTIES FOLDER test ) diff --git a/media/server/gstplayer/include/GstGenericPlayer.h b/media/server/gstplayer/include/GstGenericPlayer.h index 9ab42bf7d..0281fadc3 100644 --- a/media/server/gstplayer/include/GstGenericPlayer.h +++ b/media/server/gstplayer/include/GstGenericPlayer.h @@ -28,6 +28,7 @@ #include "IGstGenericPlayer.h" #include "IGstGenericPlayerPrivate.h" #include "IGstInitialiser.h" +#include "IGstProfiler.h" #include "IGstProtectionMetadataHelperFactory.h" #include "IGstSrc.h" #include "IGstWrapper.h" @@ -58,11 +59,11 @@ class GstGenericPlayerFactory : public IGstGenericPlayerFactory */ static std::weak_ptr m_factory; - std::unique_ptr createGstGenericPlayer( - IGstGenericPlayerClient *client, IDecryptionService &decryptionService, MediaType type, - const VideoRequirements &videoRequirements, bool isLive, - const std::shared_ptr &rdkGstreamerUtilsWrapperFactory, - const std::shared_ptr &gstProfilerFactory) override; + std::unique_ptr + createGstGenericPlayer(IGstGenericPlayerClient *client, IDecryptionService &decryptionService, MediaType type, + const VideoRequirements &videoRequirements, bool isLive, + const std::shared_ptr + &rdkGstreamerUtilsWrapperFactory) override; }; /** diff --git a/media/server/gstplayer/interface/IGstProfiler.h b/media/server/gstplayer/include/IGstProfiler.h similarity index 100% rename from media/server/gstplayer/interface/IGstProfiler.h rename to media/server/gstplayer/include/IGstProfiler.h diff --git a/media/server/gstplayer/interface/IGstGenericPlayer.h b/media/server/gstplayer/interface/IGstGenericPlayer.h index f367b6d82..6664f4b66 100644 --- a/media/server/gstplayer/interface/IGstGenericPlayer.h +++ b/media/server/gstplayer/interface/IGstGenericPlayer.h @@ -28,7 +28,6 @@ #include "IDataReader.h" #include "IDecryptionService.h" #include "IGstGenericPlayerClient.h" -#include "IGstProfiler.h" #include "IHeartbeatHandler.h" #include "IMediaPipeline.h" #include "IRdkGstreamerUtilsWrapper.h" @@ -54,25 +53,21 @@ class IGstGenericPlayerFactory static std::shared_ptr getFactory(); /** - * @brief Creates an IGstGenericPlayer object. + * @brief Creates a IGstGenericPlayer object. * - * @param[in] client : The gstreamer player client. - * @param[in] decryptionService : The decryption service. - * @param[in] type : The media type the gstreamer player shall support. - * @param[in] videoRequirements : The video requirements for the playback. - * @param[in] isLive : Indicates if the media is live. - * @param[in] rdkGstreamerUtilsWrapperFactory : The rdk gstreamer utils wrapper factory. - * @param[in] gstProfilerFactory : The gst profiler factory. Defaults to nullptr; when null, the - * concrete factory falls back to IGstProfilerFactory::getFactory(). - * This avoids evaluating the real singleton at mocked call sites. + * @param[in] client : The gstreamer player client. + * @param[in] decryptionService : The decryption service. + * @param[in] type : The media type the gstreamer player shall support. + * @param[in] videoRequirements : The video requirements for the playback. + * @param[in] isLive : Indicates if the media is live. * * @retval the new player instance or null on error. */ - virtual std::unique_ptr createGstGenericPlayer( - IGstGenericPlayerClient *client, IDecryptionService &decryptionService, MediaType type, - const VideoRequirements &videoRequirements, bool isLive, - const std::shared_ptr &rdkGstreamerUtilsWrapperFactory, - const std::shared_ptr &gstProfilerFactory = nullptr) = 0; + virtual std::unique_ptr + createGstGenericPlayer(IGstGenericPlayerClient *client, IDecryptionService &decryptionService, MediaType type, + const VideoRequirements &videoRequirements, bool isLive, + const std::shared_ptr + &rdkGstreamerUtilsWrapperFactory) = 0; }; class IGstGenericPlayer diff --git a/media/server/gstplayer/source/GstGenericPlayer.cpp b/media/server/gstplayer/source/GstGenericPlayer.cpp index a374ffb0e..9d6e94f6d 100644 --- a/media/server/gstplayer/source/GstGenericPlayer.cpp +++ b/media/server/gstplayer/source/GstGenericPlayer.cpp @@ -84,8 +84,7 @@ std::shared_ptr IGstGenericPlayerFactory::getFactory() std::unique_ptr GstGenericPlayerFactory::createGstGenericPlayer( IGstGenericPlayerClient *client, IDecryptionService &decryptionService, MediaType type, const VideoRequirements &videoRequirements, bool isLive, - const std::shared_ptr &rdkGstreamerUtilsWrapperFactory, - const std::shared_ptr &gstProfilerFactory) + const std::shared_ptr &rdkGstreamerUtilsWrapperFactory) { std::unique_ptr gstPlayer; @@ -110,21 +109,10 @@ std::unique_ptr GstGenericPlayerFactory::createGstGenericPlay throw std::runtime_error("Cannot create RdkGstreamerUtilsWrapper"); } - // Fall back to the default profiler factory if the caller explicitly - // passed nullptr. IGstProfilerFactory::getFactory() can itself return - // nullptr on allocation failure, so re-check afterwards and fail with - // a clear error rather than passing null on and reporting the generic - // "No gst profiler factory provided" message from the constructor. - auto resolvedGstProfilerFactory = gstProfilerFactory ? gstProfilerFactory : IGstProfilerFactory::getFactory(); - if (!resolvedGstProfilerFactory) - { - throw std::runtime_error("Cannot obtain default IGstProfilerFactory"); - } - gstPlayer = std::make_unique< GstGenericPlayer>(client, decryptionService, type, videoRequirements, isLive, gstWrapper, glibWrapper, rdkGstreamerUtilsWrapper, IGstInitialiser::instance(), std::make_unique(), - IGstSrcFactory::getFactory(), resolvedGstProfilerFactory, + IGstSrcFactory::getFactory(), IGstProfilerFactory::getFactory(), common::ITimerFactory::getFactory(), std::make_unique(client, gstWrapper, glibWrapper, rdkGstreamerUtilsWrapper, diff --git a/scripts/gtest/build_and_run_tests.py b/scripts/gtest/build_and_run_tests.py index e982c036c..3ad119f44 100644 --- a/scripts/gtest/build_and_run_tests.py +++ b/scripts/gtest/build_and_run_tests.py @@ -30,60 +30,11 @@ valgrindOutput = "valgrind_report" valgrindIgnore = "rialto.supp" -# Argparse type helper: only accept integers >= 1. Values like 0 (make treats -# as unlimited jobs) or negatives would defeat the purpose of --jobs on -# memory-constrained hosts. -def positiveInt(value): - try: - intValue = int(value) - except ValueError: - raise argparse.ArgumentTypeError(f"must be an integer >= 1, got {value!r}") - if intValue < 1: - raise argparse.ArgumentTypeError(f"must be >= 1, got {value}") - return intValue - -# Argparse type helper: only accept a non-empty, relative path that does not -# escape the working directory, either lexically or via symlinks. This -# protects the destructive --clean path from unsafe --output values (empty -# string, absolute path, ".", "..", or a symlink into another location). -# Fails at parse time with a clean CLI message rather than a Python traceback. -def safeRelativePath(value): - if not value: - raise argparse.ArgumentTypeError("must not be empty") - if os.path.isabs(value): - raise argparse.ArgumentTypeError(f"must be a relative path, got {value!r}") - normalised = os.path.normpath(value) - # os.path.normpath collapses internal ".." segments, so a normalised - # path escapes the working directory iff it equals ".." or starts with - # ".." + separator. We also reject paths that collapse to "." (e.g. "." - # itself or "foo/.."), since those would target the working directory. - if normalised == ".": - raise argparse.ArgumentTypeError(f"must not be the working directory, got {value!r}") - if normalised == ".." or normalised.startswith(".." + os.sep): - raise argparse.ArgumentTypeError(f"must not escape the working directory, got {value!r}") - # Lexical checks above are not enough on their own: a symlink whose - # target is outside the working directory would still let "rm -rf" - # escape. Resolve the path (following existing symlinks) and confirm - # it lives strictly under cwd. If the path does not exist yet, realpath - # returns the canonical absolute form, which is still safe. - cwd = os.path.realpath(os.getcwd()) - resolved = os.path.realpath(normalised) - if resolved == cwd: - raise argparse.ArgumentTypeError(f"must not resolve to the working directory, got {value!r}") - try: - common = os.path.commonpath([resolved, cwd]) - except ValueError: - # e.g. different drives on Windows - raise argparse.ArgumentTypeError(f"must resolve inside the working directory, got {value!r}") - if common != cwd: - raise argparse.ArgumentTypeError(f"must resolve inside the working directory, got {value!r}") - return normalised - # Get the arguments supported by the googletest script def getGenericArguments(argParser, suiteInfo): # Get arguments - argParser.add_argument("-o", "--output", type=safeRelativePath, default=safeRelativePath("build"), - help="Location to write the build files to (default 'build'). Must be a relative path.") + argParser.add_argument("-o", "--output", default="build", + help="Location to write the build files to (default 'build').") argParser.add_argument("-f", "--file", nargs='?', const="", help="Write the build and test output to a file (default '" + getDefaultResultsOutputFileName() + ".log') \n" \ + "Valgrind output also written to file, default file name only \n" \ @@ -112,8 +63,6 @@ def getGenericArguments(argParser, suiteInfo): + "Note: Valgrind can only write output to one source (log or xml). \n" \ + "Note: Requires version valgrind 3.17.0+ installed. \n") argParser.add_argument("-cov", "--coverage", action='store_true', help="Generates the full coverage report") - argParser.add_argument("-j", "--jobs", type=positiveInt, default=multiprocessing.cpu_count(), - help="Number of parallel make jobs, must be >= 1 (default: number of CPUs).") # Builds and runs googletests for the given suites def buildAndRunGTests(args, f, buildDefines, suitesToRun): @@ -124,16 +73,12 @@ def buildAndRunGTests(args, f, buildDefines, suitesToRun): os.environ["RIALTO_CONSOLE_LOG"] = "1" # Set env variable to enable debug prints os.environ["RIALTO_DEBUG"] = "5" + # Enable profiler for all test suites. + os.environ["PROFILER_ENABLED"] = "true" - # Clean if required. args['output'] is validated at CLI parse time by - # safeRelativePath(), which enforces both lexical and symlink-resolved - # containment inside the working directory, so "rm -rf" here cannot - # target an unsafe location. The "--" terminator prevents any value - # starting with "-" (which the validator would accept as a relative - # path) from being misinterpreted by rm as an option like - # --no-preserve-root. + # Clean if required if args['clean'] == True: - executeCmd = ["rm", "-rf", "--", args['output'], valgrindOutput + ".log"] + executeCmd = ["rm", "-rf", args['output'], valgrindOutput + ".log"] runcmd(executeCmd, cwd=os.getcwd()) # Get xml output file name if any @@ -147,7 +92,7 @@ def buildAndRunGTests(args, f, buildDefines, suitesToRun): # Build the test executables if args['noBuild'] == False: - buildTargets(suitesToRun, buildDefines, args['output'], f, args['valgrind'], args['coverage'], args['jobs']) + buildTargets(suitesToRun, buildDefines, args['output'], f, args['valgrind'], args['coverage']) # Run the tests with the optional settings if args['noTest'] == False: @@ -156,7 +101,7 @@ def buildAndRunGTests(args, f, buildDefines, suitesToRun): # Build the target executables -def buildTargets (suites, buildDefines, outputDir, resultsFile, debug, coverage, jobs=multiprocessing.cpu_count()): +def buildTargets (suites, buildDefines, outputDir, resultsFile, debug, coverage): # Run cmake cmakeCmd = ["cmake", "-B", outputDir] for define in buildDefines: @@ -168,7 +113,7 @@ def buildTargets (suites, buildDefines, outputDir, resultsFile, debug, coverage, runcmd(cmakeCmd, cwd=os.getcwd()) # Make targets - jarg = "-j" + str(jobs) + jarg = "-j" + str(multiprocessing.cpu_count()) makeCmd = ["make", jarg] for key in suites: makeCmd.append(suites[key]["suite"]) diff --git a/tests/unittests/media/server/gstplayer/genericPlayer/CreateTest.cpp b/tests/unittests/media/server/gstplayer/genericPlayer/CreateTest.cpp index ded6bd8b7..5e9a6f454 100644 --- a/tests/unittests/media/server/gstplayer/genericPlayer/CreateTest.cpp +++ b/tests/unittests/media/server/gstplayer/genericPlayer/CreateTest.cpp @@ -152,20 +152,21 @@ TEST_F(RialtoServerCreateGstGenericPlayerTest, FactoryCreatesObject) expectSetSignalCallbacks(); expectSetUri(); expectCheckPlaySink(); - expectCreateProfiler(); EXPECT_CALL(*m_gstWrapperMock, gstElementSetState(&m_pipeline, GST_STATE_READY)) .WillOnce(Return(GST_STATE_CHANGE_SUCCESS)); + EXPECT_CALL(*m_gstWrapperMock, gstObjectRef(&m_pipeline)).WillOnce(Return(&m_pipeline)); + std::shared_ptr factory = firebolt::rialto::server::IGstGenericPlayerFactory::getFactory(); ASSERT_NE(factory, nullptr); - auto player{factory->createGstGenericPlayer(&m_gstPlayerClient, m_decryptionServiceMock, m_type, m_videoReq, m_kIsLive, - m_rdkGstreamerUtilsWrapperFactoryMock, m_gstProfilerFactoryMock)}; + auto player{factory->createGstGenericPlayer(&m_gstPlayerClient, m_decryptionServiceMock, m_type, m_videoReq, + m_kIsLive, m_rdkGstreamerUtilsWrapperFactoryMock)}; EXPECT_NE(player, nullptr); // Destroy expectations EXPECT_CALL(*m_gstWrapperMock, gstBusSetSyncHandler(nullptr, nullptr, nullptr, nullptr)); EXPECT_CALL(*m_gstWrapperMock, gstElementSetState(_, GST_STATE_NULL)).WillOnce(Return(GST_STATE_CHANGE_SUCCESS)); - EXPECT_CALL(*m_gstWrapperMock, gstObjectUnref(_)).Times(2); + EXPECT_CALL(*m_gstWrapperMock, gstObjectUnref(_)).Times(3); EXPECT_CALL(*m_glibWrapperMock, gThreadPoolStopUnusedThreads()); player.reset(); diff --git a/tests/unittests/media/server/main/mediaPipeline/LoadTest.cpp b/tests/unittests/media/server/main/mediaPipeline/LoadTest.cpp index 2cc75dee5..0e961781b 100644 --- a/tests/unittests/media/server/main/mediaPipeline/LoadTest.cpp +++ b/tests/unittests/media/server/main/mediaPipeline/LoadTest.cpp @@ -47,7 +47,7 @@ TEST_F(RialtoServerMediaPipelineLoadTest, Success) mainThreadWillEnqueueTaskAndWait(); mainThreadWillEnqueueTask(); EXPECT_CALL(*m_gstPlayerFactoryMock, - createGstGenericPlayer(_, _, m_type, VideoRequirementsMatcher(m_videoReq), m_kIsLive, _, _)) + createGstGenericPlayer(_, _, m_type, VideoRequirementsMatcher(m_videoReq), m_kIsLive, _)) .WillOnce(Return(ByMove(std::move(m_gstPlayer)))); EXPECT_CALL(*m_mediaPipelineClientMock, notifyNetworkState(NetworkState::BUFFERING)); @@ -62,7 +62,7 @@ TEST_F(RialtoServerMediaPipelineLoadTest, CreateGstPlayerFailure) { mainThreadWillEnqueueTaskAndWait(); EXPECT_CALL(*m_gstPlayerFactoryMock, - createGstGenericPlayer(_, _, m_type, VideoRequirementsMatcher(m_videoReq), m_kIsLive, _, _)) + createGstGenericPlayer(_, _, m_type, VideoRequirementsMatcher(m_videoReq), m_kIsLive, _)) .WillOnce(Return(ByMove(nullptr))); EXPECT_CALL(*m_mediaPipelineClientMock, notifyNetworkState(_)).Times(0); diff --git a/tests/unittests/media/server/main/mediaPipeline/base/MediaPipelineTestBase.cpp b/tests/unittests/media/server/main/mediaPipeline/base/MediaPipelineTestBase.cpp index 6b73ecccc..abfe2537e 100644 --- a/tests/unittests/media/server/main/mediaPipeline/base/MediaPipelineTestBase.cpp +++ b/tests/unittests/media/server/main/mediaPipeline/base/MediaPipelineTestBase.cpp @@ -86,7 +86,7 @@ void MediaPipelineTestBase::loadGstPlayer() { mainThreadWillEnqueueTaskAndWait(); mainThreadWillEnqueueTask(); - EXPECT_CALL(*m_gstPlayerFactoryMock, createGstGenericPlayer(_, _, _, _, _, _, _)) + EXPECT_CALL(*m_gstPlayerFactoryMock, createGstGenericPlayer(_, _, _, _, _, _)) .WillOnce(DoAll(SaveArg<0>(&m_gstPlayerCallback), Return(ByMove(std::move(m_gstPlayer))))); EXPECT_CALL(*m_mediaPipelineClientMock, notifyNetworkState(NetworkState::BUFFERING)); diff --git a/tests/unittests/media/server/mocks/gstplayer/GstGenericPlayerFactoryMock.h b/tests/unittests/media/server/mocks/gstplayer/GstGenericPlayerFactoryMock.h index 23b903ecb..0139f54da 100644 --- a/tests/unittests/media/server/mocks/gstplayer/GstGenericPlayerFactoryMock.h +++ b/tests/unittests/media/server/mocks/gstplayer/GstGenericPlayerFactoryMock.h @@ -36,8 +36,7 @@ class GstGenericPlayerFactoryMock : public IGstGenericPlayerFactory (IGstGenericPlayerClient * client, IDecryptionService &decryptionService, MediaType type, const VideoRequirements &videoRequirements, bool isLive, const std::shared_ptr - &rdkGstreamerUtilsWrapperFactory, - const std::shared_ptr &gstProfilerFactory), + &rdkGstreamerUtilsWrapperFactory), (override)); }; } // namespace firebolt::rialto::server