A single test case failed during automated CI builds. We could not reproduce it locally. The failure was caused by something strange. What should be the "default" instance of a POJO was being populated with random values. "This shouldn't be happening," I thought, "The instance is final and immutable."
public class CreativeLabels {
private static final CreativeLabels UNLABELED_INSTANCE = new CreativeLabels();
private final Collection rejectedCreatives;
private final Collection allowedCreatives;
private final Collection limitedCreatives;
"I must be misreading the code. It must be accessing some other instance, not UNLABELED_INSTANCE, which is designed to be immutable and empty by default," I thought. I was wrong. Fields marked as "final" can be modified. We got bit by
this problem today.
setFinalStatic(Boolean.class.getField("FALSE"), true);
System.out.format("Everything is %s", false); // "Everything is true"
My team and I advocate immutability here because it makes code easier to understand. It's harder to create immutable objects in Java, but code is read more often then it is written, so the effort is worth while. But we also have done some things that makes code behave in unintuitive ways, such as using this pattern:
public class SomePojo {
@JsonProperty("id")
private String id;
@JsonProperty("label")
private String label;
public String getId() { return this.id; }
public String getLabel() { return this.label; }
}
Above, we design model objects meant to be deserialized from json in production use. They have private fields that cannot be modified (right?). During unit test, we write code like this:
public class SomePojoTest {
@Mock
private SomePojoService someService;
@Test
public void verifyPojoUse() {
SomePojo testData = new SomePojo();
ReflectionTestUtils.setField(testData, "id", "123");
when(someService.read()).thenReturn(testData);
assertEquals("123", testData.getId());
}
}
In other words, we use reflection tricks to update "hidden" fields during testing to set up mock data. For clarification, this is not code I'd write. Looking at this code, I believe the developer's intention was that this will make the code safer by making the object effectively immutable, except during unit testing where magic is allowed. It seems too good to be true - this POJO has very little lines of code, but is still "immutable". I certainly did not argue against it when I saw someone write code like this. How do you argue against less code? And this practice has proliferated for a while. Today, we were bitten by it. In some unit test somewhere, a test case has used reflection to change the fields of a globally shared immutable object. This, in turn, caused other test cases to fail. Anytime you use magic to modify something that advertise itself as "immutable", you are planting a time bomb for future developers. In a way, you are lying to your developers. You're saying "Hey, this object should be immutable... (except when it is convenient for me that they are mutable)."
In retrospect, I should've put a stop to this practice as soon as I saw it. It was my fault that I didn't. It is against my nature to argue over what seems to be trivial things. But the longer I've coded in a team environment, the more I realized a principled attitude is necessary to ensure a consistent environment that ultimately benefits a large team.
No comments:
Post a Comment