https://issues.dlang.org/show_bug.cgi?id=13454

On Monday, 27 April 2015 at 07:28:28 UTC, Martin Nowak wrote:
> https://issues.dlang.org/show_bug.cgi?id=13454

Great, lets compromise both unittest correctness and convienience at the same time. Can we please just close the issue as `RESOLVED WONTFIX`? Compiling tests of dependencies pretty much never causes any notable slowdown. Trying to be smart about it does harm the users and contributors of the library though.

On Monday, 27 April 2015 at 09:22:48 UTC, Dicebot wrote:
> Compiling tests of dependencies pretty much never causes any notable slowdown.

This thread doesn't support that view, see the first post.

On Monday, 27 April 2015 at 10:15:20 UTC, Kagamin wrote:
> On Monday, 27 April 2015 at 09:22:48 UTC, Dicebot wrote:
>> Compiling tests of dependencies pretty much never causes any notable slowdown.
>
> This thread doesn't support that view, see the first post.

Which part exactly? I only see comparisons for compiling AND running tests for dependencies. And it is usually running which causes the slowdown.

On 4/27/15 6:20 AM, Dicebot wrote:
> On Monday, 27 April 2015 at 10:15:20 UTC, Kagamin wrote:
>> On Monday, 27 April 2015 at 09:22:48 UTC, Dicebot wrote:
>>> Compiling tests of dependencies pretty much never causes any notable
>>> slowdown.
>>
>> This thread doesn't support that view, see the first post.
>
> Which part exactly? I only see comparisons for compiling AND running
> tests for dependencies. And it is usually running which causes the
> slowdown.

The problem is as follows:

1. Unit tests for some library are written for that library. They are written to run tests during unit tests of that library only (possibly with certain requirements of environment, including build lines, or expectations of system resource availability).
2. People who import that library's modules are not trying to test the library, they are trying to test their code.
3. The library runs its unit tests for templates in this case, not the other unit tests in the module (those would be run in that module's unit tests, which means you'd have to recompile the library to run unit tests). Most often, unit tests in templates are only expected to be defined by the library.

I think ketmar had the most reasonable request here -- only compile/run unit tests in templates for the module that defines the template.

If you want to create a unit test that runs for your specialized template version, there should be a way to do that, but it should be opt-in. I envision something like this:

unittest
{
	import somelib.somemod : SomeModTemplate, unittest
	SomeModTemplate!MyLocalType t; // runs templated unit tests
}

If you look in RedBlackTree, I use templated unit tests to great effect (found about 4-5 compiler bugs that way). But I have a lot of machinery around the unit tests to try and only test unit tests on integral types. It would be nice to not have to write all that shit.

And BTW, I didn't even get it right, I forgot about other parameters to the RBTree, necessitating a PR later.

-Steve

On Monday, 27 April 2015 at 11:30:04 UTC, Steven Schveighoffer wrote:
> The problem is as follows:
>
> 1. Unit tests for some library are written for that library. They are written to run tests during unit tests of that library only (possibly with certain requirements of environment, including build lines, or expectations of system resource availability).

By the way, a unittest-related issue still stands in DMD 2.067.1 for RedBlackTree:
https://issues.dlang.org/show_bug.cgi?id=12246

A similar matter got resolved quickly for BinaryHeap:
https://issues.dlang.org/show_bug.cgi?id=12245

BinaryHeap's case was handled by putting the container's unittests into "debug(BinaryHeap)".

RedBlackTree's case is controlled via "debug(RBDoChecks)" (formerly "version(RBDoChecks)").  The difference is the presence of a "version(unittest) debug = RBDoChecks;" line.  This looks inconsistent.

For RedBlackTree, compile the following with or without -unittest option and run for a visible difference in speed (runs momentarily or for a few seconds):
-----
import std.container;
void main() {
	auto t = redBlackTree!int;
	foreach (i; 0..3000) t.insert(i);
}
-----

Ivan Kazmenko.

On 4/27/15 10:30 AM, Ivan Kazmenko wrote:
> On Monday, 27 April 2015 at 11:30:04 UTC, Steven Schveighoffer wrote:
>> The problem is as follows:
>>
>> 1. Unit tests for some library are written for that library. They are
>> written to run tests during unit tests of that library only (possibly
>> with certain requirements of environment, including build lines, or
>> expectations of system resource availability).
>
> By the way, a unittest-related issue still stands in DMD 2.067.1 for
> RedBlackTree:
> https://issues.dlang.org/show_bug.cgi?id=12246
>
> A similar matter got resolved quickly for BinaryHeap:
> https://issues.dlang.org/show_bug.cgi?id=12245
>
> BinaryHeap's case was handled by putting the container's unittests into
> "debug(BinaryHeap)".
>
> RedBlackTree's case is controlled via "debug(RBDoChecks)" (formerly
> "version(RBDoChecks)").  The difference is the presence of a
> "version(unittest) debug = RBDoChecks;" line.  This looks inconsistent.
>
> For RedBlackTree, compile the following with or without -unittest option
> and run for a visible difference in speed (runs momentarily or for a few
> seconds):
> -----
> import std.container;
> void main() {
>      auto t = redBlackTree!int;
>      foreach (i; 0..3000) t.insert(i);
> }
> -----
>
> Ivan Kazmenko.

It's an anecdotal fix. I remember arguing over the change to debug, that was done for purity (pure functions need debug mode to print out something, which rbdochecks will do if there is an issue), but I can't find the conversation.

But someone else will complain that when they try to debug their code, adding -debug to the command line debugs RBTree's algorithm (similarly to how they complained BinaryHeap was doing this). This really should only EVER run during phobos unit tests.

I don't know how to fix this properly without something like I outlined above, or without doing some global version(PhobosUnitTests) hack.

In fact, I don't agree with the BinaryHeap change. At this point, phobos unit tests are NOT testing the binary heap structure. Sure that makes user code run faster, but at the cost of never testing it even when it should be tested.

-Steve

On Monday, 27 April 2015 at 15:29:05 UTC, Steven Schveighoffer wrote:
> On 4/27/15 10:30 AM, Ivan Kazmenko wrote:
>> On Monday, 27 April 2015 at 11:30:04 UTC, Steven Schveighoffer wrote:
>>> The problem is as follows:
>>>
>>> 1. Unit tests for some library are written for that library. They are
>>> written to run tests during unit tests of that library only (possibly
>>> with certain requirements of environment, including build lines, or
>>> expectations of system resource availability).
>>
>> By the way, a unittest-related issue still stands in DMD 2.067.1 for
>> RedBlackTree:
>> https://issues.dlang.org/show_bug.cgi?id=12246
>>
>> A similar matter got resolved quickly for BinaryHeap:
>> https://issues.dlang.org/show_bug.cgi?id=12245
>>
>> BinaryHeap's case was handled by putting the container's unittests into
>> "debug(BinaryHeap)".
>>
>> RedBlackTree's case is controlled via "debug(RBDoChecks)" (formerly
>> "version(RBDoChecks)").  The difference is the presence of a
>> "version(unittest) debug = RBDoChecks;" line.  This looks inconsistent.
>>
>> For RedBlackTree, compile the following with or without -unittest option
>> and run for a visible difference in speed (runs momentarily or for a few
>> seconds):
>> -----
>> import std.container;
>> void main() {
>>     auto t = redBlackTree!int;
>>     foreach (i; 0..3000) t.insert(i);
>> }
>> -----
>>
>> Ivan Kazmenko.
>
> It's an anecdotal fix. I remember arguing over the change to debug, that was done for purity (pure functions need debug mode to print out something, which rbdochecks will do if there is an issue), but I can't find the conversation.
>
> But someone else will complain that when they try to debug their code, adding -debug to the command line debugs RBTree's algorithm (similarly to how they complained BinaryHeap was doing this).

I am doing just that, right now =) .  But it's -unittest, not -debug, which triggers RBTree's expensive checks.

> This really should only EVER run during phobos unit tests.

A choice would be even better.  When a user's code is wrong, for example, a comparison function does not define a comparison in mathematical sense, the user might benefit from container's sanity checks.  From personal experience, when I implement my own sorted container and some complex logic using it, I do benefit from the container's checks sometimes.

> I don't know how to fix this properly without something like I outlined above, or without doing some global version(PhobosUnitTests) hack.
>
> In fact, I don't agree with the BinaryHeap change. At this point, phobos unit tests are NOT testing the binary heap structure. Sure that makes user code run faster, but at the cost of never testing it even when it should be tested.
>
> -Steve

Right now, it is possible to test binary heap integrity with an explicit "-debug=BinaryHeap", and it is not tested by default.

And it is mandatory to test red-black tree integrity when running any unittests involving a red-black tree, as it is tested by default.

So, from a user's point of view, I like the binary heap situation better, since it gives me a choice with a reasonable default.

How Phobos' unittests should be handled is another matter which, in my opinion, should not take away choices, and reasonable defaults too, from the user.

To me, the proposed version(PhobosUnitTests) hack looks good for the particular case.  But I don't see whether such trick scales well for the whole ecosystem of libraries.

Ivan Kazmenko.

> To me, the proposed version(PhobosUnitTests) hack looks good for the particular case.  But I don't see whether such trick scales well for the whole ecosystem of libraries.

Another approach that might work specifically for containers is to pass as an additional template parameter whether the particular RedBlackTree must continuously check its integrity or not.  That way, the parameter can be explicitly set to "doCheck" from the unittest instantiations, default to "doNotCheck" for other code, and be enabled on a case-by-case basis when debugging.

Again, I don't see whether it fits the larger picture, but still think it's worth to consider.

Ivan Kazmenko.

On 4/27/15 12:00 PM, Ivan Kazmenko wrote:

> Right now, it is possible to test binary heap integrity with an explicit
> "-debug=BinaryHeap", and it is not tested by default.

Right, but phobos unit tests are not doing that. If we added a version(unittest) debug = BinaryHeap; to fix that problem there, then you would still be complaining ;)

-Steve