Hacker News new | ask | show | jobs
by chris_wot 950 days ago
Not sure why I’m getting downvoted so badly, because by its very nature refactoring should t change the functionality of the system. If you have functional unit tests that are failing, then something has changed and your refactor has changed the behaviour of the system!
1 comments
tsimionescu 950 days ago
It is very common for unit tests to be white-box testing, and thus to depend significantly on internal details of a class.

Say, when unit testing a list class, a test might call the add function and then assert that the length field has changed appropriately.

Then, if you change the list to calculate length on demand instead of keeping a length field, your test will now fail even thought the behavior has not actually changed.

This is a somewhat silly example, but it is very common for unit tests to depend on implementation details. And note that this is not about private VS public methods/fields. The line between implementation details and public API is fuzzy and depends on the larger use of the unit within the system.

link
zmgsabst 950 days ago
The behavior has changed:

Checking length is now a function call and not a cached variable — a change in call signature and runtime performance.

Consumers of your list class are going to have to update their code (eg, that checks the list length) and your test successfully notified you of that breaking API change.

link
tsimionescu 950 days ago
Then any code change is a breaking API change and the term API is meaningless. If the compiler replaces a conditional jump + a move with a conditional move, it has now changed the total length of my code and affected its performance, and now users will have to adjust their code accordingly.

The API of a piece of code is a convention, sometimes compiler enforced, typically not entirely. If that convention is broken, it's good that tests fail. If changes outside that convention break tests, then it's pure overhead to repair those tests.

As a side note, the length check is not necessarily no longer cached just because the variable is no longer visible to that test. Perhaps the custom list implementation was replaced with a wrapper around java.ArrayList, so the length field is no longer accessible.

link
chris_wot 949 days ago
In your example, if your code relied on the previous functionality, then surely you could get a nasty bug?
link