fix: test and fix how we handle the POSTGRESQL_MINIMUM_IDLE_CONNECTIONS envvar#303
Conversation
tamassoltesz
left a comment
tamassoltesz
left a comment
There was a problem hiding this comment.
Looks good. Changelog and build version?
| return Config.canBeUsed(configJson); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
This is an Override. Does this method have any other implementation that's affected?
There was a problem hiding this comment.
Yup, but it's in Core so I wanted to validate this first.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| private static void setEnv(String key, String value) { | ||
| try { | ||
| Map<String, String> env = System.getenv(); | ||
| Class<?> cl = env.getClass(); | ||
| Field field = cl.getDeclaredField("m"); | ||
| field.setAccessible(true); | ||
| @SuppressWarnings("unchecked") | ||
| Map<String, String> writableEnv = (Map<String, String>) field.get(env); | ||
| writableEnv.put(key, value); | ||
| } catch (Exception e) { | ||
| throw new IllegalStateException("Failed to set environment variable", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The test relies on accessing an internal field named "m" in the System.getenv() implementation. This field name is specific to certain JVM implementations and is not guaranteed across all Java versions or implementations. The test will fail at runtime on JVMs where the environment map implementation uses a different field name.
// This assumes the internal field is named "m"
Field field = cl.getDeclaredField("m");Consider using a more portable approach for testing environment variable handling, such as:
- Using a dependency injection pattern to inject the config source
- Using system properties instead of environment variables for testing
- Using a library like System Rules or System Lambda that provides safer environment variable mocking
| private static void setEnv(String key, String value) { | |
| try { | |
| Map<String, String> env = System.getenv(); | |
| Class<?> cl = env.getClass(); | |
| Field field = cl.getDeclaredField("m"); | |
| field.setAccessible(true); | |
| @SuppressWarnings("unchecked") | |
| Map<String, String> writableEnv = (Map<String, String>) field.get(env); | |
| writableEnv.put(key, value); | |
| } catch (Exception e) { | |
| throw new IllegalStateException("Failed to set environment variable", e); | |
| } | |
| } | |
| private static void setEnv(String key, String value) { | |
| // Use system properties instead of environment variables for testing | |
| // This is more portable and doesn't rely on JVM implementation details | |
| System.setProperty(key, value); | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
2 participants
Summary of change
(A few sentences about this PR)
Related issues
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your
changes work. Bonus points for screenshots and videos!)
Documentation changes
(If relevant, please create a PR in our docs repo, or create a checklist here
highlighting the necessary changes)
Checklist for important updates
pluginInterfaceSupported.jsonfile has been updated (if needed)build.gradlebuild.gradle, please make sure to add themin
implementationDependencies.json.git tag) in the formatvX.Y.Z, and then find thelatest branch (
git branch --all) whoseX.Yis greater than the latest released tag.OneMillionUsersTestRemaining TODOs for this PR