From 8deaa809cd561676c47dfc799f025858e193928a Mon Sep 17 00:00:00 2001 From: Atanas Atanasov Date: Wed, 18 Dec 2024 17:22:00 +0200 Subject: [PATCH] test: add automatic unit test detection of missing configuration to environment variable mapping (#421) Signed-off-by: Atanas Atanasov --- ...rverMappedConfigSourceInitializerTest.java | 99 ++++++++--- simulator/build.gradle.kts | 2 + ...atorMappedConfigSourceInitializerTest.java | 158 ++++++++++++++++-- 3 files changed, 221 insertions(+), 38 deletions(-) diff --git a/server/src/test/java/com/hedera/block/server/config/ServerMappedConfigSourceInitializerTest.java b/server/src/test/java/com/hedera/block/server/config/ServerMappedConfigSourceInitializerTest.java index ee853ee89..cbf56264c 100644 --- a/server/src/test/java/com/hedera/block/server/config/ServerMappedConfigSourceInitializerTest.java +++ b/server/src/test/java/com/hedera/block/server/config/ServerMappedConfigSourceInitializerTest.java @@ -20,19 +20,24 @@ import static org.assertj.core.api.Assertions.fail; import com.swirlds.common.metrics.config.MetricsConfig; +import com.swirlds.common.metrics.platform.prometheus.PrometheusConfig; import com.swirlds.config.api.ConfigData; import com.swirlds.config.api.ConfigProperty; import com.swirlds.config.extensions.sources.ConfigMapping; import com.swirlds.config.extensions.sources.MappedConfigSource; +import java.lang.System.Logger.Level; import java.lang.reflect.Field; import java.lang.reflect.RecordComponent; import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; import java.util.Optional; import java.util.Queue; import java.util.Set; import java.util.function.Predicate; +import java.util.stream.Collectors; import java.util.stream.Stream; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -42,20 +47,37 @@ * Tests for {@link ServerMappedConfigSourceInitializer}. */ class ServerMappedConfigSourceInitializerTest { + private static final System.Logger LOGGER = + System.getLogger(ServerMappedConfigSourceInitializerTest.class.getName()); private static final ConfigMapping[] SUPPORTED_MAPPINGS = { + // Consumer Config new ConfigMapping("consumer.timeoutThresholdMillis", "CONSUMER_TIMEOUT_THRESHOLD_MILLIS"), + + // Persistence Config new ConfigMapping("persistence.storage.liveRootPath", "PERSISTENCE_STORAGE_LIVE_ROOT_PATH"), new ConfigMapping("persistence.storage.archiveRootPath", "PERSISTENCE_STORAGE_ARCHIVE_ROOT_PATH"), new ConfigMapping("persistence.storage.type", "PERSISTENCE_STORAGE_TYPE"), new ConfigMapping("persistence.storage.compression", "PERSISTENCE_STORAGE_COMPRESSION"), new ConfigMapping("persistence.storage.compressionLevel", "PERSISTENCE_STORAGE_COMPRESSION_LEVEL"), + + // Service Config new ConfigMapping("service.delayMillis", "SERVICE_DELAY_MILLIS"), + + // Mediator Config new ConfigMapping("mediator.ringBufferSize", "MEDIATOR_RING_BUFFER_SIZE"), new ConfigMapping("mediator.type", "MEDIATOR_TYPE"), + + // Notifier Config new ConfigMapping("notifier.ringBufferSize", "NOTIFIER_RING_BUFFER_SIZE"), + + // Producer Config new ConfigMapping("producer.type", "PRODUCER_TYPE"), + + // Server Config new ConfigMapping("server.maxMessageSizeBytes", "SERVER_MAX_MESSAGE_SIZE_BYTES"), new ConfigMapping("server.port", "SERVER_PORT"), + + // Prometheus Config (externally managed, but we need this mapping) new ConfigMapping("prometheus.endpointEnabled", "PROMETHEUS_ENDPOINT_ENABLED"), new ConfigMapping("prometheus.endpointPortNumber", "PROMETHEUS_ENDPOINT_PORT_NUMBER") }; @@ -71,27 +93,39 @@ class ServerMappedConfigSourceInitializerTest { * - all fields in all config classes are annotated with the * {@link ConfigProperty} annotation. * - a mapping for all fields in all config classes is present in the - * {@link ServerMappedConfigSourceInitializer#MAPPINGS()}. + * {@link ServerMappedConfigSourceInitializer#MAPPINGS()}, as defined + * in {@link #allManagedConfigDataTypes()}. * * @param config parameterized, config class to test + * @param fieldNamesToExclude parameterized, fields to exclude from the test */ - @Disabled("This test is disabled because it will start passing only after #285 gets implemented") @ParameterizedTest - @MethodSource("allConfigDataTypes") - void testVerifyAllFieldsInRecordsAreMapped(final Class config) { + @MethodSource("allManagedConfigDataTypes") + void testVerifyAllFieldsInRecordsAreMapped( + final Class config, final List fieldNamesToExclude) { + final String configClassName = config.getSimpleName(); if (!config.isAnnotationPresent(ConfigData.class)) { - fail("Class %s is missing the ConfigData annotation! All config classes MUST have that annotation present!" - .formatted(config.getSimpleName())); + fail( + "Class [%s] is missing the ConfigData annotation! All config classes MUST have that annotation present!" + .formatted(configClassName)); } else { final ConfigData configDataAnnotation = config.getDeclaredAnnotation(ConfigData.class); final String prefix = configDataAnnotation.value(); - for (final RecordComponent recordComponent : config.getRecordComponents()) { + final RecordComponent[] fieldsToVerify = Arrays.stream(config.getRecordComponents()) + .filter(recordComponent -> !fieldNamesToExclude.contains(recordComponent.getName())) + .toArray(RecordComponent[]::new); + LOGGER.log( + Level.INFO, + "Checking fields %s\nfor config class [%s]." + .formatted(Arrays.toString(fieldsToVerify), configClassName)); + for (final RecordComponent recordComponent : fieldsToVerify) { + final String fieldName = recordComponent.getName(); if (!recordComponent.isAnnotationPresent(ConfigProperty.class)) { fail( - "Field %s in %s is missing the ConfigProperty annotation! All fields in config classes MUST have that annotation present!" - .formatted(recordComponent.getName(), config.getSimpleName())); + "Field [%s] in [%s] is missing the ConfigProperty annotation! All fields in config classes MUST have that annotation present!" + .formatted(fieldName, configClassName)); } else { - final String expectedMappedName = "%s.%s".formatted(prefix, recordComponent.getName()); + final String expectedMappedName = "%s.%s".formatted(prefix, fieldName); final Optional matchingMapping = Arrays.stream(SUPPORTED_MAPPINGS) .filter(mapping -> mapping.mappedName().equals(expectedMappedName)) .findFirst(); @@ -99,8 +133,8 @@ void testVerifyAllFieldsInRecordsAreMapped(final Class config) .isNotNull() .withFailMessage( "Field [%s] in [%s] is not present in the environment variable mappings! Expected config key [%s] to be present and to be mapped to [%s]", - recordComponent.getName(), - config.getSimpleName(), + fieldName, + configClassName, expectedMappedName, transformToEnvVarConvention(expectedMappedName)) .isPresent(); @@ -153,12 +187,6 @@ void testVerifyAllSupportedMappingsAreAddedToInstance() throws ReflectiveOperati } } - private static String transformToEnvVarConvention(final String input) { - String underscored = input.replace(".", "_"); - String resolved = underscored.replaceAll("(? extractConfigMappings() throws ReflectiveOperationException { final Field configMappings = MappedConfigSource.class.getDeclaredField("configMappings"); @@ -171,13 +199,38 @@ private static Queue extractConfigMappings() throws ReflectiveOpe } } - private static Stream allConfigDataTypes() { + private static String transformToEnvVarConvention(final String input) { + final String underscored = input.replace(".", "_"); + final String resolved = underscored.replaceAll("(? allManagedConfigDataTypes() { // Add any classes that should be excluded from the test for any reason in the set below - // MetricsConfig is not supported by us + // MetricsConfig is not managed by us. final Set> excluded = Set.of(MetricsConfig.class); - return new BlockNodeConfigExtension() + + // Add any classes that should be partially checked in the map below + // for example, we do not manage PrometheusConfig, but we need the endpointEnabled and endpointPortNumber + // mappings to be present in our scope, so we exclude all other fields. We must do the strategy + // of exclusion and not inclusion because fields in the config classes can be added or removed, we want + // to detect that. + final Entry, List> prometheusFieldsToExclude = + Map.entry(PrometheusConfig.class, List.of("endpointMaxBacklogAllowed")); + final Map, List> fieldNamesToExcludeForConfig = + Map.ofEntries(prometheusFieldsToExclude); + + // Here are all the config classes that we need to check mappings for + final Set> allRegisteredRecordsFilteredWithExcluded = new BlockNodeConfigExtension() .getConfigDataTypes().stream() .filter(configType -> !excluded.contains(configType)) - .map(Arguments::of); + .collect(Collectors.toSet()); + + // Here we return all config classes that we need to check mappings for and a list of field names to + // exclude from the test for the given config class. If the list is empty, all fields will be checked. + return allRegisteredRecordsFilteredWithExcluded.stream().map(config -> { + final List fieldsToExclude = fieldNamesToExcludeForConfig.getOrDefault(config, List.of()); + return Arguments.of(config, fieldsToExclude); + }); } } diff --git a/simulator/build.gradle.kts b/simulator/build.gradle.kts index 61d9da975..040b539b4 100644 --- a/simulator/build.gradle.kts +++ b/simulator/build.gradle.kts @@ -38,8 +38,10 @@ mainModuleInfo { testModuleInfo { requires("org.junit.jupiter.api") + requires("org.junit.jupiter.params") requires("org.mockito") requires("org.mockito.junit.jupiter") + requires("org.assertj.core") requiresStatic("com.github.spotbugs.annotations") requires("com.google.protobuf") } diff --git a/simulator/src/test/java/com/hedera/block/simulator/config/SimulatorMappedConfigSourceInitializerTest.java b/simulator/src/test/java/com/hedera/block/simulator/config/SimulatorMappedConfigSourceInitializerTest.java index 343a59410..0fa9081c6 100644 --- a/simulator/src/test/java/com/hedera/block/simulator/config/SimulatorMappedConfigSourceInitializerTest.java +++ b/simulator/src/test/java/com/hedera/block/simulator/config/SimulatorMappedConfigSourceInitializerTest.java @@ -16,18 +16,36 @@ package com.hedera.block.simulator.config; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.assertj.core.api.Assertions.assertThat; +import com.swirlds.common.metrics.config.MetricsConfig; +import com.swirlds.common.metrics.platform.prometheus.PrometheusConfig; +import com.swirlds.config.api.ConfigData; +import com.swirlds.config.api.ConfigProperty; import com.swirlds.config.extensions.sources.ConfigMapping; import com.swirlds.config.extensions.sources.MappedConfigSource; +import java.lang.System.Logger.Level; import java.lang.reflect.Field; +import java.lang.reflect.RecordComponent; +import java.util.Arrays; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Optional; import java.util.Queue; +import java.util.Set; import java.util.function.Predicate; -import org.junit.jupiter.api.BeforeAll; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.assertj.core.api.Assertions; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; class SimulatorMappedConfigSourceInitializerTest { + private static final System.Logger LOGGER = + System.getLogger(SimulatorMappedConfigSourceInitializerTest.class.getName()); private static final ConfigMapping[] SUPPORTED_MAPPINGS = { // gRPC configuration new ConfigMapping("grpc.serverAddress", "GRPC_SERVER_ADDRESS"), @@ -50,15 +68,70 @@ class SimulatorMappedConfigSourceInitializerTest { new ConfigMapping("generator.startBlockNumber", "GENERATOR_START_BLOCK_NUMBER"), new ConfigMapping("generator.endBlockNumber", "GENERATOR_END_BLOCK_NUMBER"), - // Prometheus configuration + // Prometheus configuration (externally managed, but we need this mapping) new ConfigMapping("prometheus.endpointEnabled", "PROMETHEUS_ENDPOINT_ENABLED"), new ConfigMapping("prometheus.endpointPortNumber", "PROMETHEUS_ENDPOINT_PORT_NUMBER") }; - private static MappedConfigSource toTest; - @BeforeAll - static void setUp() { - toTest = SimulatorMappedConfigSourceInitializer.getMappedConfigSource(); + /** + * This test aims to verify the state of all config extensions we have + * added. These configs are the ones that are returned from + * {@link SimulatorConfigExtension#getConfigDataTypes()}. This test will + * verify: + *
+     *     - all added config classes are annotated with the {@link ConfigData}
+     *       annotation.
+     *     - all fields in all config classes are annotated with the
+     *       {@link ConfigProperty} annotation.
+     *     - a mapping for all fields in all config classes is present in the
+     *       {@link SimulatorMappedConfigSourceInitializer#MAPPINGS()}, as
+     *       defined in {@link #allManagedConfigDataTypes()}.
+     * 
+ * @param config parameterized, config class to test + * @param fieldNamesToExclude parameterized, fields to exclude from the test + */ + @ParameterizedTest + @MethodSource("allManagedConfigDataTypes") + void testVerifyAllFieldsInRecordsAreMapped( + final Class config, final List fieldNamesToExclude) { + final String configClassName = config.getSimpleName(); + if (!config.isAnnotationPresent(ConfigData.class)) { + Assertions.fail( + "Class [%s] is missing the ConfigData annotation! All config classes MUST have that annotation present!" + .formatted(configClassName)); + } else { + final ConfigData configDataAnnotation = config.getDeclaredAnnotation(ConfigData.class); + final String prefix = configDataAnnotation.value(); + final RecordComponent[] fieldsToVerify = Arrays.stream(config.getRecordComponents()) + .filter(recordComponent -> !fieldNamesToExclude.contains(recordComponent.getName())) + .toArray(RecordComponent[]::new); + LOGGER.log( + Level.INFO, + "Checking fields %s\nfor config class [%s]." + .formatted(Arrays.toString(fieldsToVerify), configClassName)); + for (final RecordComponent recordComponent : fieldsToVerify) { + final String fieldName = recordComponent.getName(); + if (!recordComponent.isAnnotationPresent(ConfigProperty.class)) { + Assertions.fail( + "Field [%s] in [%s] is missing the ConfigProperty annotation! All fields in config classes MUST have that annotation present!" + .formatted(fieldName, configClassName)); + } else { + final String expectedMappedName = "%s.%s".formatted(prefix, fieldName); + final Optional matchingMapping = Arrays.stream(SUPPORTED_MAPPINGS) + .filter(mapping -> mapping.mappedName().equals(expectedMappedName)) + .findFirst(); + assertThat(matchingMapping) + .isNotNull() + .withFailMessage( + "Field [%s] in [%s] is not present in the environment variable mappings! Expected config key [%s] to be present and to be mapped to [%s]", + fieldName, + configClassName, + expectedMappedName, + transformToEnvVarConvention(expectedMappedName)) + .isPresent(); + } + } + } } /** @@ -71,29 +144,84 @@ static void setUp() { * SimulatorMappedConfigSourceInitializer#MAPPINGS} to make this pass. */ @Test - void test_VerifyAllSupportedMappingsAreAddedToInstance() throws ReflectiveOperationException { + void testVerifyAllSupportedMappingsAreAddedToInstance() throws ReflectiveOperationException { final Queue actual = extractConfigMappings(); - assertEquals(SUPPORTED_MAPPINGS.length, actual.size()); + // fail if the actual and this test have a different number of mappings + assertThat(SUPPORTED_MAPPINGS.length) + .withFailMessage( + "The number of supported mappings has changed! Please update the test to reflect the change.\nRUNTIME_MAPPING: %s\nTEST_MAPPING: %s", + actual, Arrays.toString(SUPPORTED_MAPPINGS)) + .isEqualTo(actual.size()); + // test this test against actual for (final ConfigMapping current : SUPPORTED_MAPPINGS) { final Predicate predicate = cm -> current.mappedName().equals(cm.mappedName()) && current.originalName().equals(cm.originalName()); - assertTrue( - actual.stream().anyMatch(predicate), - () -> "when testing for: [%s] it is not contained in mappings of the actual initialized object %s" - .formatted(current, actual)); + assertThat(actual.stream().anyMatch(predicate)) + .withFailMessage( + "When testing for: [%s] it is not contained in mappings of the actual initialized object %s", + current, actual) + .isTrue(); + } + + // test actual against this test + for (final ConfigMapping current : actual) { + final Predicate predicate = + cm -> current.mappedName().equals(cm.mappedName()) + && current.originalName().equals(cm.originalName()); + assertThat(Arrays.stream(SUPPORTED_MAPPINGS).anyMatch(predicate)) + .withFailMessage( + "When testing for: [%s] it is not contained in mappings of this test %s", current, actual) + .isTrue(); } } + @SuppressWarnings("unchecked") private static Queue extractConfigMappings() throws ReflectiveOperationException { final Field configMappings = MappedConfigSource.class.getDeclaredField("configMappings"); try { configMappings.setAccessible(true); - return (Queue) configMappings.get(toTest); + return (Queue) + configMappings.get(SimulatorMappedConfigSourceInitializer.getMappedConfigSource()); } finally { configMappings.setAccessible(false); } } + + private static String transformToEnvVarConvention(final String input) { + final String underscored = input.replace(".", "_"); + final String resolved = underscored.replaceAll("(? allManagedConfigDataTypes() { + // Add any classes that should be excluded from the test for any reason in the set below + // MetricsConfig is not managed by us. + final Set> excluded = Set.of(MetricsConfig.class); + + // Add any classes that should be partially checked in the map below + // for example, we do not manage PrometheusConfig, but we need the endpointEnabled and endpointPortNumber + // mappings to be present in our scope, so we exclude all other fields. We must do the strategy + // of exclusion and not inclusion because fields in the config classes can be added or removed, we want + // to detect that. + final Entry, List> prometheusFieldsToExclude = + Map.entry(PrometheusConfig.class, List.of("endpointMaxBacklogAllowed")); + final Map, List> fieldNamesToExcludeForConfig = + Map.ofEntries(prometheusFieldsToExclude); + + // Here are all the config classes that we need to check mappings for + final Set> allRegisteredRecordsFilteredWithExcluded = new SimulatorConfigExtension() + .getConfigDataTypes().stream() + .filter(configType -> !excluded.contains(configType)) + .collect(Collectors.toSet()); + + // Here we return all config classes that we need to check mappings for and a list of field names to + // exclude from the test for the given config class. If the list is empty, all fields will be checked. + return allRegisteredRecordsFilteredWithExcluded.stream().map(config -> { + final List fieldsToExclude = fieldNamesToExcludeForConfig.getOrDefault(config, List.of()); + return Arguments.of(config, fieldsToExclude); + }); + } }