Improve testCleanTerms_completelyCleansArray #18
Conversation
lucaswerkmeister
force-pushed
the
clean-inmemory
branch
2 times, most recently
from
05b3166
to
3acc58e
Compare
The two calls to acquireTermIds seemed to be accidental complexity so I changed to only one call. I also removed access to the private field as this is surprising and breaks automated refactoring. New approach has class define explicitly what can be accessed. ... Just realized: Since the tested class is an in memory implementation meant for tests and implements a package private interface, it is probably better placed in tests/TestDoubles or similar rather than src/. Lets clean that up in some follow up.
AlaaSarhan
force-pushed
the
completelyCleansArray
branch
from
ff7cf21
to
6812213
Compare
|
Also rebased and improved this based on both suggestions: do not bind to internal implementation, check that terms are empty when there no leaf nodes (term texts) not just that the top array is an empty array. |
|
oh I see.. maybe I misunderstood the diiscussion I thought from your comment that you wanted to actually test that the array is empty as in Okay will have a second in-depth look and adjust it acoordingly |
|
Renamed one test to actually tests For a test-double, testing its behavior through covering the helper methods only is enough and probably preferred (as doubles usually do tricks internally when pretending to implement the behavior they "mock") |
AlaaSarhan
dismissed
their stale review
I've made changes so my approval is irrelevant now
| // if there's any leaf element in terms then it's not empty, otherwise we consider | ||
| // the terms array empty even if it had some sub-arrays that are also empty by | ||
| // this definition | ||
| array_walk_recursive( |
There was a problem hiding this comment.
This is going to walk the whole array all the time :/ But given this is probably only going to be used in tests, I guess that's bearable.
There was a problem hiding this comment.
yeah this array isn't expected to contain more than tens of elements for testing purposes (hopefully).
There was a problem hiding this comment.
Is the old version that used assertEmpty on the terms field broken? This looks like a behavior change to me. I already asked about it on the PR that motivated this change and got no answer.
There was a problem hiding this comment.
I might have got lost with the conversation and misunderstood it (though I doubt).
You may ignore this PR entirely or just revert my changes (I can do it on command ;) if you think it is completely irrelevant to the original improvement/discussion. Sorry to have wasted your time in that case :)
There was a problem hiding this comment.
The first version did
assertEmpty( $terms )
Then I changed it do
assertTrue( empty( $terms ) )
AFAIK that is equivalent. But Lucas said I broke it. Your change here suggests you think it was indeed broken. Where did it get broken?
There was a problem hiding this comment.
Let's not sweat this one anymore.. it is so trivial anyway. Either merge or kill at will please
The two calls to acquireTermIds seemed to be accidental
complexity so I changed to only one call.
I also removed access to the private field as this is surprising
and breaks automated refactoring. New approach has class define
explicitly what can be accessed.
... Just realized:
Since the tested class is an in memory implementation meant
for tests and implements a package private interface, it is
probably better placed in tests/TestDoubles or similar rather
than src/. Lets clean that up in some follow up.