webcompere/system-stubs

System Stubs v2.x suppresses at test time any modifications made to the real System.getenv `Map` by the application

Closed this issue · 6 comments

yeikel commented

We are currently using 1.2.0 and are trying to upgrade to v2 to solve issues like #83

Unfortunately, my application not only reads but also mutates the environment as part of its critical path

As System Stub has its own copy of the environment, the mutations that we make are then not propagated correctly to the System stub context which makes my tests fail

The following reproducer passes in 1.2.0 but fails in all v2 versions

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import uk.org.webcompere.systemstubs.environment.EnvironmentVariables;
import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension;

import java.lang.reflect.Field;
import java.util.Map;

import static org.assertj.core.api.Assertions.assertThat;

@ExtendWith(SystemStubsExtension.class)
public class ReproducerTest {

    static class MyClassUnderTest{

        private static Field getAccessibleField(final Class<?> clazz, final String fieldName) throws NoSuchFieldException {
            final Field field = clazz.getDeclaredField(fieldName);
            field.setAccessible(true);
            return field;
        }

        static void methodWithInjection(){
            assertThat(System.getenv("Hello")).isEqualTo("World"); // Passes
            injectEnvironmentVariables(Map.of("ENV1", "123"));
        }

        private static void injectEnvironmentVariables(final Map<String, String> entries) {
            try {
                final Class<?> processEnvironment = Class.forName("java.lang.ProcessEnvironment");

                final Field unmodifiableEnvironmentField = getAccessibleField(processEnvironment, "theUnmodifiableEnvironment");
                final Object unmodifiableEnvironment = unmodifiableEnvironmentField.get(null);
                injectIntoUnmodifiableMap(unmodifiableEnvironment, entries);

                try {
                    final Field caseInsensitiveEnvironmentField = getAccessibleField(processEnvironment, "theCaseInsensitiveEnvironment");
                    @SuppressWarnings("unchecked") final Map<String, String> caseInsensitiveEnvironment = (Map<String, String>) caseInsensitiveEnvironmentField.get(null);
                    caseInsensitiveEnvironment.putAll(entries);
                } catch (final NoSuchFieldException ignored) {
                    // theCaseInsensitiveEnvironment is only available on Windows
                }
            } catch (final Exception e) {
                throw new UnsupportedOperationException(e);
            }
        }

        private static void injectIntoUnmodifiableMap(final Object map, final Map<String, String> entries) throws ReflectiveOperationException {
            final Class<?> unmodifiableMap = Class.forName("java.util.Collections$UnmodifiableMap");
            final Field field = getAccessibleField(unmodifiableMap, "m");
            @SuppressWarnings("unchecked") final Map<String, String> obj = (Map<String, String>) field.get(map);
            obj.putAll(entries);
        }
    }

    @Test
    void test(final EnvironmentVariables env) {
        env.set("Hello","World");

        MyClassUnderTest.methodWithInjection();

        assertThat(System.getenv("ENV1")).isEqualTo("123"); //fails in v2 but passes in v1
    }

}
ashleyfrieze commented

Some notes on this one.

Firstly, it's quite unusual for an application to modify the environment the way the OP is describing. This should result in a reflection error in later versions of Java.

In this article we cover all the test frameworks that can be used to replace environment variables at test time, including System Stubs, and we also demonstrate the hack around the Map which System Stubs v1.x uses (among others), including the JVM runtime parameters to continue to allow this, even though later versions of Java prevent it by default.

By design System Stubs v2.x doesn't hack the actual Map in the system, but maintains a separate map with the test settings in, and intercepts calls to fetch environment variables.

Under the circumstances, therefore, I think it's reasonable to add the JUnit 5.11 fix to the 1.2.x release train in order to provide a compatible version.

In general, the advice to other readers of this issue is don't modify your environment variables at runtime in production code. However, System Stubs v1.2.x along with the JVM parameters is still viable in later versions of Java, though compatibilty with deprecated Java APIs is not guaranteed.

yeikel commented

Some notes on this one.

Firstly, it's quite unusual for an application to modify the environment the way the OP is describing. This should result in a reflection error in later versions of Java.

In this article we cover all the test frameworks that can be used to replace environment variables at test time, including System Stubs, and we also demonstrate the hack around the Map which System Stubs v1.x uses (among others), including the JVM runtime parameters to continue to allow this, even though later versions of Java prevent it by default.

By design System Stubs v2.x doesn't hack the actual Map in the system, but maintains a separate map with the test settings in, and intercepts calls to fetch environment variables.

Under the circumstances, therefore, I think it's reasonable to add the JUnit 5.11 fix to the 1.2.x release train in order to provide a compatible version.

In general, the advice to other readers of this issue is don't modify your environment variables at runtime in production code. However, System Stubs v1.2.x along with the JVM parameters is still viable in later versions of Java, though compatibilty with deprecated Java APIs is not guaranteed.

Thank you for taking the time to explain the background

We indeed faced challenges with Java 17 with this code and we got around passing JVM arguments to still allow it

ie

--add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED

There is a lot of legacy code that is using this functionality but as you highlighted passing the arguments is not a good solution long term

ashleyfrieze commented

Those add opens in production code worry me ;)

I've merged #85

This weekend I'll look at maybe adding in the TestNG plugin to the 1.2.x release train... and then I'll try to get v1.2.1 released soon.

Please bear with me.

yeikel commented

Those add opens in production code worry me ;)

I've merged #85

This weekend I'll look at maybe adding in the TestNG plugin to the 1.2.x release train... and then I'll try to get v1.2.1 released soon.

Please bear with me.

They worry me as well. Ultimately we'd like to get away from using the environment in general but it'll take time

No rush, and thank you for back-porting the fix

ashleyfrieze commented

I've released v1.2.1 this weekend. This should be available in maven central within a few hours.

For clarity, the restriction here is that we want the combination of test-time environment variable changes to be mixed with the application itself modifying environment variables via the old reflection trick. The v1 version of the code works with reflection, the v2 version uses a different environment variables map substituted into the getenv calls during test time.

yeikel commented

I've released v1.2.1 this weekend. This should be available in maven central within a few hours.

For clarity, the restriction here is that we want the combination of test-time environment variable changes to be mixed with the application itself modifying environment variables via the old reflection trick. The v1 version of the code works with reflection, the v2 version uses a different environment variables map substituted into the getenv calls during test time.

I can confirm that the release fixed the issue with JUnit 5. Thank you!