Skip to content

test: standardize JVM system property save/restore pattern in kubernetes-client-api tests #7669

@manusa

Description

@manusa

Background

PR #7667 enabled <reuseForks>true</reuseForks> in kubernetes-client-api and surfaced a latent bug in CertUtilsTest: an @AfterEach that called System.setProperties(snapshot) was wiping the entire JVM property table (including os.name, user.home, etc.) and leaking into sibling tests.

While auditing the rest of the module, the System.setProperties(snapshot) antipattern is not present elsewhere — but tests that mutate system properties are split across multiple correct-but-inconsistent idioms, with at least one fragile case that relies on every test method remembering to wrap in try/finally.

We want to converge on a single idiom to:

  • Reduce the chance that a future test added under fork reuse leaks state into a sibling
  • Make the cleanup behavior obvious and uniform to readers
  • Avoid adding a new test-scope dependency (e.g. JUnit Pioneer) — we want to reduce dependencies, not add them

Standard pattern

Use a per-property save/restore in @BeforeEach/@AfterEach, storing originals (including null) in a Map<String, String>. The post-#7667 CertUtilsTest and the existing ConfigTest are the canonical examples.

private static final String[] MANAGED_PROPERTIES = { /* keys mutated by this test */ };
private Map<String, String> originalProperties;

@BeforeEach
void storeProperties() {
  originalProperties = new HashMap<>();
  for (String property : MANAGED_PROPERTIES) {
    originalProperties.put(property, System.getProperty(property));
  }
}

@AfterEach
void restoreProperties() {
  for (String property : MANAGED_PROPERTIES) {
    String original = originalProperties.get(property);
    if (original == null) {
      System.clearProperty(property);
    } else {
      System.setProperty(property, original);
    }
  }
}

Tasks

  • Audit the full codebase for any test class under **/src/test/** that mutates JVM system properties (System.setProperty, System.clearProperty, System.getProperties().remove(...), or any remaining System.setProperties(...) calls). The targets list below is based on a survey of kubernetes-client-api and a partial scan of other modules; expand it with anything else turned up. Useful starting commands:
    • rg -n --glob '**/src/test/**' 'System\.setProperties\('
    • rg -n --glob '**/src/test/**' 'System\.setProperty\(|System\.clearProperty\('
    • rg -n --glob '**/src/test/**' 'System\.getProperties\(\)'
  • kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigSourcePrecedenceTest.java — outer @BeforeEach sets kubeconfig with no @AfterEach; relies on every test using try/finally. Highest-risk target.
  • kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigProxySourceTest.java — mixed idioms across nested classes; pick one and apply uniformly.
  • kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigRefreshTest.java — migrate inline try/finally blocks to @BeforeEach/@AfterEach.
  • kubernetes-client/src/test/java/io/fabric8/kubernetes/client/utils/internal/URLFromServiceUtilTest.java — same migration; lower priority since fork reuse is not enabled in kubernetes-client yet.
  • Migrate any additional classes identified by the audit task above.

Out of scope

  • Adopting third-party libraries (JUnit Pioneer, System Stubs, System Lambda). Explicitly avoided to keep test dependencies minimal.
  • Repo-wide migration outside kubernetes-client-api/kubernetes-client. Other modules can be migrated opportunistically when touched, unless the audit reveals fork-reuse-relevant risk.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions