Use new macros in existing surface area, so that classes marked as final don't have virtual methods. (#5389)

* Use new macros in existing surface area, so that classes marked as final don't have virtual methods.

* Update doc comments.

* Use DOXYGEN_PREDEFINED to expand only the macros we want expanded.

* Add the compile definition for more projects.

* Address PR feedback.

* Make TestableTokenCache a friend class of TokenCache.

cmake-modules/AzureDoxygen.cmake

@@ -26,6 +26,14 @@ function(generate_documentation PROJECT_NAME PROJECT_VERSION)
         # classes and enums directly into the documentation.
         set(DOXYGEN_INLINE_SOURCES NO)
         set(DOXYGEN_MARKDOWN_ID_STYLE GITHUB)
+        # Used to correctly expand macros like _azure_NON_FINAL_FOR_TESTS when generating docs.
+        # Using EXPAND_ONLY_PREDEF to limit macro expansion to the macros specified with the PREDEFINED tags.
+        set(DOXYGEN_MACRO_EXPANSION YES)
+        set(EXPAND_ONLY_PREDEF YES)
+        set(DOXYGEN_PREDEFINED 
+            _azure_NON_FINAL_FOR_TESTS=final
+            _azure_VIRTUAL_FOR_TESTS=
+            )
         # Skip generating docs for json, test, samples, and private files.
         set(DOXYGEN_EXCLUDE_PATTERNS
             json.hpp

sdk/core/azure-core-amqp/CMakeLists.txt

@@ -193,6 +193,7 @@ az_rtti_setup(
 if(BUILD_TESTING)
   # define a symbol that enables some test hooks in code
   add_compile_definitions(TESTING_BUILD)
+  add_compile_definitions(_azure_TESTING_BUILD)
   
   if (NOT AZ_ALL_LIBRARIES)
     include(AddGoogleTest)

sdk/core/azure-core-tracing-opentelemetry/CMakeLists.txt

@@ -97,6 +97,7 @@ endif()
 if(BUILD_AZURE_CORE_TRACING_OPENTELEMETRY AND BUILD_TESTING)
   # define a symbol that enables some test hooks in code
   add_compile_definitions(TESTING_BUILD)
+  add_compile_definitions(_azure_TESTING_BUILD)
   
   if (NOT AZ_ALL_LIBRARIES)
     include(AddGoogleTest)

sdk/core/azure-core/CMakeLists.txt

@@ -96,6 +96,7 @@ set(
     inc/azure/core/internal/json/json_optional.hpp
     inc/azure/core/internal/json/json_serializable.hpp
     inc/azure/core/internal/strings.hpp
+    inc/azure/core/internal/test_hooks.hpp
     inc/azure/core/internal/tracing/service_tracing.hpp
     inc/azure/core/internal/tracing/tracing_impl.hpp
     inc/azure/core/internal/unique_handle.hpp
@@ -205,6 +206,7 @@ az_rtti_setup(
 if(BUILD_TESTING)
   # define a symbol that enables some test hooks in code
   add_compile_definitions(TESTING_BUILD)
+  add_compile_definitions(_azure_TESTING_BUILD)
   
   if (NOT AZ_ALL_LIBRARIES)
     include(AddGoogleTest)

sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp

@@ -16,6 +16,7 @@
 #include "azure/core/http/transport.hpp"
 #include "azure/core/internal/http/http_sanitizer.hpp"
 #include "azure/core/internal/http/user_agent.hpp"
+#include "azure/core/internal/test_hooks.hpp"
 #include "azure/core/uuid.hpp"
 
 #include <atomic>
@@ -30,6 +31,14 @@
 #include <utility>
 #include <vector>
 
+#if defined(_azure_TESTING_BUILD)
+// Define the class used from tests to validate retry policy
+namespace Azure { namespace Core { namespace Test {
+  class RetryPolicyTest;
+  class RetryLogic;
+}}} // namespace Azure::Core::Test
+#endif
+
 /**
  * A function that should be implemented and linked to the end-user application in order to override
  * an HTTP transport implementation provided by Azure SDK with custom implementation.
@@ -363,11 +372,13 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
     /**
      * @brief HTTP retry policy.
      */
-    class RetryPolicy
+    class RetryPolicy _azure_NON_FINAL_FOR_TESTS : public HttpPolicy {
-#if !defined(TESTING_BUILD)
+
-        final
+#if defined(_azure_TESTING_BUILD)
+      friend class Azure::Core::Test::RetryPolicyTest;
+      friend class Azure::Core::Test::RetryLogic;
 #endif
-        : public HttpPolicy {
+
     private:
       RetryOptions m_retryOptions;
 
@@ -402,14 +413,14 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
        */
       static int32_t GetRetryCount(Context const& context);
 
-    protected:
+    private:
-      virtual bool ShouldRetryOnTransportFailure(
+      _azure_VIRTUAL_FOR_TESTS bool ShouldRetryOnTransportFailure(
           RetryOptions const& retryOptions,
           int32_t attempt,
           std::chrono::milliseconds& retryAfter,
           double jitterFactor = -1) const;
 
-      virtual bool ShouldRetryOnResponse(
+      _azure_VIRTUAL_FOR_TESTS bool ShouldRetryOnResponse(
           RawResponse const& response,
           RetryOptions const& retryOptions,
           int32_t attempt,

sdk/core/azure-core/inc/azure/core/internal/test_hooks.hpp

@@ -0,0 +1,47 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+/**
+ * @file
+ * @brief This file is used to define internal-only macros that are used to control the behavior of
+ * the Azure SDK when running tests, to allow mocking within tests.
+ * The macros in this file should NOT be used by anyone outside this repo.
+ */
+
+#pragma once
+
+// When testing is enabled, we want to make sure that certain classes are not final, so that we can
+// mock it.
+#if defined(_azure_TESTING_BUILD)
+
+/**
+ * @brief If we are testing, we want to make sure that classes are not final, by default.
+ */
+#if !defined(_azure_NON_FINAL_FOR_TESTS)
+#define _azure_NON_FINAL_FOR_TESTS
+#endif
+
+/**
+ * @brief If we are testing, we want to make sure methods can be made virtual, for mocking.
+ */
+#if !defined(_azure_VIRTUAL_FOR_TESTS)
+#define _azure_VIRTUAL_FOR_TESTS virtual
+#endif
+
+#else
+
+/**
+ * @brief If we are not testing, we want to make sure that classes are final, by default.
+ */
+#if !defined(_azure_NON_FINAL_FOR_TESTS)
+#define _azure_NON_FINAL_FOR_TESTS final
+#endif
+
+/**
+ * @brief If we are not testing, we don't need to make methods virtual for mocking.
+ */
+#if !defined(_azure_VIRTUAL_FOR_TESTS)
+#define _azure_VIRTUAL_FOR_TESTS
+#endif
+
+#endif

sdk/identity/azure-identity/inc/azure/identity/azure_cli_credential.hpp

@@ -9,11 +9,11 @@
 #pragma once
 
 #include "azure/identity/detail/token_cache.hpp"
-#include "azure/identity/dll_import_export.hpp"
 
 #include <azure/core/credentials/credentials.hpp>
 #include <azure/core/credentials/token_credential_options.hpp>
 #include <azure/core/datetime.hpp>
+#include <azure/core/internal/test_hooks.hpp>
 
 #include <chrono>
 #include <string>
@@ -56,11 +56,7 @@ namespace Azure { namespace Identity {
    * @brief Enables authentication to Microsoft Entra ID using Azure CLI to obtain an access
    * token.
    */
-  class AzureCliCredential
+  class AzureCliCredential _azure_NON_FINAL_FOR_TESTS : public Core::Credentials::TokenCredential {
-#if !defined(_azure_TESTING_BUILD)
-      final
-#endif
-      : public Core::Credentials::TokenCredential {
 
 #if defined(_azure_TESTING_BUILD)
     friend class Azure::Identity::Test::AzureCliTestCredential;

sdk/identity/azure-identity/inc/azure/identity/detail/token_cache.hpp

@@ -10,6 +10,7 @@
 #pragma once
 
 #include <azure/core/credentials/credentials.hpp>
+#include <azure/core/internal/test_hooks.hpp>
 
 #include <chrono>
 #include <functional>
@@ -19,26 +20,30 @@
 #include <string>
 #include <tuple>
 
+#if defined(_azure_TESTING_BUILD)
+// Define the class used from tests to validate retry policy
+namespace Azure { namespace Identity { namespace Test {
+  class TestableTokenCache;
+}}} // namespace Azure::Identity::Test
+#endif
+
 namespace Azure { namespace Identity { namespace _detail {
   /**
    * @brief Access token cache.
    *
    */
-  class TokenCache
+  class TokenCache _azure_NON_FINAL_FOR_TESTS {
-#if !defined(TESTING_BUILD)
+
-      final
+#if defined(_azure_TESTING_BUILD)
+    friend class Azure::Identity::Test::TestableTokenCache;
 #endif
-  {
+
-#if !defined(TESTING_BUILD)
   private:
-#else
-  protected:
-#endif
     // A test hook that gets invoked before cache write lock gets acquired.
-    virtual void OnBeforeCacheWriteLock() const {};
+    _azure_VIRTUAL_FOR_TESTS void OnBeforeCacheWriteLock() const {};
 
     // A test hook that gets invoked before item write lock gets acquired.
-    virtual void OnBeforeItemWriteLock() const {};
+    _azure_VIRTUAL_FOR_TESTS void OnBeforeItemWriteLock() const {};
 
     struct CacheKey
     {
@@ -63,7 +68,6 @@ namespace Azure { namespace Identity { namespace _detail {
     mutable std::map<CacheKey, std::shared_ptr<CacheValue>, CacheKeyComparator> m_cache;
     mutable std::shared_timed_mutex m_cacheMutex;
 
-  private:
     TokenCache(TokenCache const&) = delete;
     TokenCache& operator=(TokenCache const&) = delete;
 

sdk/identity/azure-identity/inc/azure/identity/dll_import_export.hpp

@@ -38,16 +38,6 @@
 
 #undef AZ_IDENTITY_BUILT_AS_DLL
 
-#if defined(_azure_TESTING_BUILD)
-#if !defined(_azure_VIRTUAL_FOR_TESTS)
-#define _azure_VIRTUAL_FOR_TESTS virtual
-#endif
-#else
-#if !defined(_azure_VIRTUAL_FOR_TESTS)
-#define _azure_VIRTUAL_FOR_TESTS
-#endif
-#endif
-
 /**
  * @brief Azure SDK abstractions.
  *

sdk/keyvault/CMakeLists.txt

@@ -10,6 +10,7 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON)
 if(BUILD_TESTING)
   # define a symbol that enables some test hooks in code
   add_compile_definitions(TESTING_BUILD)
+  add_compile_definitions(_azure_TESTING_BUILD)
 endif()
 
 add_subdirectory(azure-security-keyvault-keys)

sdk/keyvault/azure-security-keyvault-certificates/CMakeLists.txt

@@ -103,6 +103,7 @@ generate_documentation(azure-security-keyvault-certificates ${AZ_LIBRARY_VERSION
 if(BUILD_TESTING)
   # define a symbol that enables some test hooks in code
   add_compile_definitions(TESTING_BUILD)
+  add_compile_definitions(_azure_TESTING_BUILD)
 
   if (NOT AZ_ALL_LIBRARIES OR FETCH_SOURCE_DEPS)
     include(AddGoogleTest)

sdk/keyvault/azure-security-keyvault-certificates/inc/azure/keyvault/certificates/certificate_client.hpp

@@ -16,6 +16,7 @@
 #include <azure/core/context.hpp>
 #include <azure/core/http/http.hpp>
 #include <azure/core/internal/http/pipeline.hpp>
+#include <azure/core/internal/test_hooks.hpp>
 #include <azure/core/response.hpp>
 
 #include <memory>
@@ -33,11 +34,7 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Certificat
    *
    * @details The client supports retrieving KeyVaultCertificate.
    */
-  class CertificateClient
+  class CertificateClient _azure_NON_FINAL_FOR_TESTS {
-#if !defined(TESTING_BUILD)
-      final
-#endif
-  {
     friend class CreateCertificateOperation;
 
 #if defined(TESTING_BUILD)

sdk/keyvault/azure-security-keyvault-keys/inc/azure/keyvault/keys/key_client.hpp

@@ -17,6 +17,7 @@
 #include <azure/core/credentials/credentials.hpp>
 #include <azure/core/http/http.hpp>
 #include <azure/core/internal/http/pipeline.hpp>
+#include <azure/core/internal/test_hooks.hpp>
 #include <azure/core/io/body_stream.hpp>
 #include <azure/core/response.hpp>
 
@@ -34,11 +35,7 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Keys {
    * Vault. The client supports creating, retrieving, updating, deleting, purging, backing up,
    * restoring, and listing the KeyVaultKey.
    */
-  class KeyClient
+  class KeyClient _azure_NON_FINAL_FOR_TESTS {
-#if !defined(TESTING_BUILD)
-      final
-#endif
-  {
   protected:
     // Using a shared pipeline for a client to share it with LRO (like delete key)
     /** @brief the base URL for this keyvault instance. */

sdk/keyvault/azure-security-keyvault-secrets/inc/azure/keyvault/secrets/secret_client.hpp

@@ -18,6 +18,7 @@
 
 #include <azure/core/http/http.hpp>
 #include <azure/core/internal/http/pipeline.hpp>
+#include <azure/core/internal/test_hooks.hpp>
 #include <azure/core/response.hpp>
 
 #include <stdint.h>
@@ -42,11 +43,7 @@ namespace Azure { namespace Security { namespace KeyVault { namespace Secrets {
    * Vault. The client supports creating, retrieving, updating, deleting, purging, backing up,
    * restoring, and listing the secret.
    */
-  class SecretClient
+  class SecretClient _azure_NON_FINAL_FOR_TESTS {
-#if !defined(TESTING_BUILD)
-      final
-#endif
-  {
 
   private:
     // Using a shared pipeline for a client to share it with LRO (like delete key)