D8588: Make VCS data container classes implicitly shared
Friedrich W. H. Kossebau
noreply at phabricator.kde.org
Wed Nov 1 10:34:47 UTC 2017
kossebau added a comment.
In https://phabricator.kde.org/D8588#162725, @brauch wrote:
> In general looks like a good idea and I admire your persistence, but do we really want 3k lines of code of tests testing copy constructors?
Yes, result smells a little insane. Story behind is that I wanted to be a good boy(tm) and cover the changes done by some unit tests. Should be simple, right? But suddenly my memory, notes and accessible code had no templates for testing normal C++ properties API, so I turned to just experimenting.
I hoped by writing out all the code I could see some patterns which could be used to unfold things into a clever use of loops, functors or lambdas. But I failed, any tricks I tried did not really reduce the total number of lines, but added complexity.
So at that point I gave up and just kept the latest version as proposed here :)
> Does that add value? It seems like it just makes any future changes harder ... It only tests that the compiler-generated code actually does what the compiler claims it will do, no? ;)
(Somehow I mapped this onto "Unit tests making future changes harder" on first reading, how comes ;) ) Actually I tested with the tests that old and new implementation does the same, especially as I was unsure what the old implementation should behave like (incl. bugs which are used as "feature") given the API dox had no promises. And then they say ideally unit tests should not know about the implementation, but simply check the API promises?
Another thing to consider: those classes have been stable for many years, so if someone ever decides to change things in their API, they still could dump these unit tests.
The test code is here now, seems to work and does not have much testtime costs (< 1s here).
So IMHO the small value still > small costs.
To: kossebau, #kdevelop
Cc: brauch, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the KDevelop-devel