2.3.5 breaks backward compatibility

[vtintillier]

Hello,

This was working before but no longer does:

class SomeTest {
    @SystemStub
    private static SystemProperties systemProperties = new MySystemProperties().set("property", "value");
}

class MySystemProperties extends SystemProperties {

    public MySystemProperties() {
        super("key1", "value1");
        this.set("key2", "value2");
    }
}

This is due to the introduction of SystemPropertiesImpl and moving the set implementation there (in #75).

It seems given how SystemPropertiesImpl is used, we could fix the issue (and the same issue mentioned here), by doing this:

    /**
     * Set a system property. If active, this will set it with {@link System#setProperty(String, String)}.
     * If not active, then this will store the property to apply when this object is part of an execution.
     * It is also possible to use {@link System#setProperty(String, String)} while this object is active,
     * but when the execution finishes, this object will be unaware of the property set, so will not set
     * it next time.
     * @param name name of the property
     * @param value value to set
     * @return this object for fluent use
     * @since 1.0.0
     */
    @Override
-    public SystemPropertiesImpl<T> set(String name, String value) {
+    public T set(String name, String value) {
        properties.setProperty(name, value);
        if (isActive()) {
            System.setProperty(name, value);
        }
-        return this;
+        return (T) this;
    }

    /**
     * Remove a property - this removes it from system properties if active, and remembers to remove it
     * while the object is active
     * @param name the name of the property to remove
     * @return <code>this</code> for fluent use
     * @since 2.1.5
     */
    @Override
-    public SystemPropertiesImpl<T> remove(String name) {
+    public T remove(String name) {
        propertiesToRemove.add(name);
        if (isActive()) {
            System.getProperties().remove(name);
        }
-        return this;
+        return (T) this;
    }

Would you agree to this?

Thanks

Thanks a lot.

[ashleyfrieze]

Hey @vtintillier - I'm happy for you to make a PR with those changes in it. I'm not sure if they'll solve your problem. You may also need SystemProperties to be a generic type which takes the subclass type as a parameter to pass down to the impl. Unless you're planning to make MySystemProperties subclass the Impl class instead.

Do you need to subclass SystemProperties in your code? What does that achieve? Composition might be easier. You can create your own system stub objects if you inherit the right interface and have a default constructor...

However, that's potentially off-topic. Please submit the PR and I'll do my best to approve and release it.

[vtintillier]

Hi @ashleyfrieze

I'm not sure if they'll solve your problem. You may also need SystemProperties to be a generic type which takes the subclass type as a parameter to pass down to the impl. Unless you're planning to make MySystemProperties subclass the Impl class instead.

In fact, in our case, we just need to get a SystemProperties instance, not our own type. Really just need to have chained methods return the public type and not the Impl one.

Do you need to subclass SystemProperties in your code? What does that achieve? Composition might be easier. You can create your own system stub objects if you inherit the right interface and have a default constructor...

I think it was historically done like that in our codebase because we migrated from the ProvideSystemProperty rule from system-rules, where we extended that JUnit 4 rule. Indeed now with SystemProperties we could just have some shared method returning a SystemProperties instead of our own class. But still method chaining would need fixing.

[ashleyfrieze]

@vtintillier - thanks for your contribution. I've initiated a release process. The 2.1.6 version should be live in a few hours.