Many places we use system properties I imagine we could use the constant value
as a default, and allow elements to individually toggle as needed for test
coverage. This provides a similar benefit without rearchitecting the codebase,
but does have the potential to miss things.
Unfortunately a lot of components can be initialized before the configuration
is read, or must span across configuration changes, so modification can be
difficult. I tend to prefer not to add test-only flags for this sort of thing
in favor of potentially more complicated tests, otherwise we risk failure to
test what we run in production. Using the system property constants as defaults
could make both testing and production better for some features.
-ck
> On Dec 31, 2019, at 5:28 PM, Matt Sicker <boa...@gmail.com> wrote:
>
> This sounds like a neat case for use of feature flags rather than
> caching system properties.
>
>> On Tue, 31 Dec 2019 at 16:23, Volkan Yazıcı <volkan.yaz...@gmail.com> wrote:
>>
>> [Thanks so much for the prompt reply @Ralph.]
>>
>> In the plugin, I have certain behavior triggered by constants:
>>
>> if (Constants.ENABLE_THREADLOCALS) {
>> return toJsonViaTla(logEvent);
>> } else {
>> return toJsonViaObjectPool(logEvent);
>> }
>>
>> To evaluate each case, I repeat my JUnit 4 tests as follows:
>>
>> <plugin>
>> <groupId>org.apache.maven.plugins</groupId>
>> <artifactId>maven-surefire-plugin</artifactId>
>> <configuration>
>> <skip>true</skip>
>> </configuration>
>> <executions>
>> <!-- TLA-enabled execution -->
>> <execution>
>> <id>tla-enabled</id>
>> <goals>
>> <goal>test</goal>
>> </goals>
>> <configuration>
>> <skip>${maven.test.skip}</skip>
>> <systemPropertyVariables>
>>
>> <log4j2.enable.threadlocals>true</log4j2.enable.threadlocals>
>> <log4j2.is.webapp>false</log4j2.is.webapp>
>> </systemPropertyVariables>
>> </configuration>
>> </execution>
>> <!-- TLA-disabled execution -->
>> <execution>
>> <id>tla-disabled</id>
>> <goals>
>> <goal>test</goal>
>> </goals>
>> <configuration>
>> <skip>${maven.test.skip}</skip>
>> <systemPropertyVariables>
>>
>> <log4j2.enable.threadlocals>false</log4j2.enable.threadlocals>
>> <log4j2.is.webapp>true</log4j2.is.webapp>
>> </systemPropertyVariables>
>> </configuration>
>> </execution>
>> </executions>
>> </plugin>
>>
>> Prior to settling on a solution with maven-surefire-plugin, I tried
>> the following failed attempt:
>>
>> class PluginTest {
>>
>> @Test
>> public void test_tla_triggered_behavior() {
>> ...
>> }
>>
>> }
>>
>> public class TlaEnabledPluginTest extends PluginTest {
>>
>> static {
>> enableTla();
>> }
>>
>> @BeforeClass
>> public static void enableTla() {
>> System.setProperty("log4j2.enable.threadlocals", "true");
>> System.setProperty("log4j2.is.webapp", "false");
>> }
>>
>> }
>>
>> public class TlaDisabledPluginTest extends PluginTest {
>>
>> static {
>> disableTla();
>> }
>>
>> @BeforeClass
>> public static void disableTla() {
>> System.setProperty("log4j2.enable.threadlocals", "false");
>> System.setProperty("log4j2.is.webapp", "true");
>> }
>>
>> }
>>
>> Putting the failure of this approach aside, it also another
>> shortcoming: one cannot execute individual @Test methods in IDE.
>>
>> Rather than all these hacks, I wish I would have received the
>> TLA-enabled flag in a Constants object placed within
>> l.core.config.Configuration that is injected into the plugin builder.
>> This way I could have reused my test methods with just different
>> Configuration parameters:
>>
>> if (config.getConstants().isTlaEnabled()) {
>> return toJsonViaTla(logEvent);
>> } else {
>> return toJsonViaObjectPool(logEvent);
>> }
>>
>> public class PluginTest {
>>
>> @Test
>> public void test_tla_triggered_behavior() {
>>
>> Configuration tlaEnabledConfiguration = new
>> DefaultConfiguration();
>> tlaEnabledConfiguration.getConstants().setTlaEnabled(true);
>> test_tla_triggered_behavior(tlaEnabledConfiguration);
>>
>> Configuration tlaDisabledConfiguration = new
>> DefaultConfiguration();
>> tlaEnabledConfiguration.getConstants().setTlaEnabled(false);
>> test_tla_triggered_behavior(tlaDisabledConfiguration);
>> }
>>
>> private void test_tla_triggered_behavior(Cofiguration config) {
>> ...
>> }
>>
>> }
>>
>> Is this sufficient to demonstrate my case?
>>
>>> On Tue, Dec 31, 2019 at 10:52 PM Ralph Goers <ralph.go...@dslextreme.com>
>>> wrote:
>>>
>>> Can you provide a PR that demonstrates what you would like to do? It is
>>> easier for me to evaluate the pros and cons of that then guessing what the
>>> implementation would look like.
>>>
>>> Ralph
>>>
>>>> On Dec 31, 2019, at 2:43 PM, Volkan Yazıcı <volkan.yaz...@gmail.com> wrote:
>>>>
>>>> A majority of the Log4j 2.0 constants are structured by means of
>>>> "public final" fields initialized via system properties. While this
>>>> serves great for accessibility, IMHO, becomes an obstacle for tests.
>>>> For instance, I have needed two maven-surefire-plugin executions for
>>>> log4j2-logstash-layout: one with TLA enabled and another one with TLA
>>>> disabled. I have tried invoking System.setProperty() to influence
>>>> these constants for tests via both "static { }" blocks and
>>>> @BeforeClass methods, but neither kicks in earlier than the Log4j
>>>> constant class initialization itself. Is it just me that couldn't get
>>>> it working or this approach indeed hampers testing? If the latter, for
>>>> Log4j 3.0, I propose using injection to pass configuration state
>>>> rather than accessing it via globals.
>>>>
>>>
>>>
>
>
>
> --
> Matt Sicker <boa...@gmail.com>
