Netflix / archaius

Library for configuration management API
Apache License 2.0
2.46k stars 485 forks source link

Refactor PropertyTest.test() to Improve Test Experience #714

Closed Codegass closed 5 months ago

Codegass commented 6 months ago

Description

When I am running the PropertyTest I notice that there is test case named as test. It does not have a proper testing name and contains two different testing targets together, which might be clearer and more maintainable if split. Currently, it initializes a service, verifies default values, changes properties, and checks the results in one go.

    public void test() throws ConfigException {
        SettableConfig config = new DefaultSettableConfig();
        DefaultPropertyFactory factory = DefaultPropertyFactory.from(config);

        MyService service = new MyService(factory);

        assertEquals(1, (int)service.value.get());
        assertEquals(2, (int)service.value2.get());

        config.setProperty("foo", "123");

        assertEquals(123, (int)service.value.get());
        assertEquals(123, (int)service.value2.get());
        // setValue() is called once when we init to 1 and twice when we set foo to 123.
        assertEquals(1, service.setValueCallsCounter.get());
    }

Suggested Refactoring

Splitting into two tests could improve clarity, they will be rename as testServiceInitializationWithDefaultProperties and testPropertyValuesUpdateAndEffect

@Test
public void testServiceInitializationWithDefaultProperties() throws ConfigException {
    SettableConfig config = new DefaultSettableConfig();
    DefaultPropertyFactory factory = DefaultPropertyFactory.from(config);

    MyService service = new MyService(factory);

    // Verify initial property values
    assertEquals(1, (int) service.value.get());
    assertEquals(2, (int) service.value2.get());
    // Verify setValue() was called once for each initialization
    assertEquals(0, service.setValueCallsCounter.get());
}
@Test
public void testPropertyValuesUpdateAndEffect() throws ConfigException {
    SettableConfig config = new DefaultSettableConfig();
    DefaultPropertyFactory factory = DefaultPropertyFactory.from(config);
    MyService service = new MyService(factory);

    // Setting up properties
    config.setProperty("foo", "123");

    // Assertions after properties update
    assertEquals(123, (int)service.value.get());
    assertEquals(123, (int)service.value2.get());
    // setValue() is called once for each property update
    assertEquals(1, service.setValueCallsCounter.get());
}

Benefits

If you don't mind, I am willing to submit the PR for the suggestion.