On Thu, Mar 22, 2018 at 10:59:56AM +0000, Atila Neves via Digitalmars-d-announce wrote:
> Blog post:
> 
> https://atilanevesoncode.wordpress.com/
[...]

I realize this is your opinion, but I disagree with them because:

1) I've found that having unittests built into the language is a big win, *especially* because I can write tests next to the code being tested. This is a big win because needing to open up a new file and finding the right place to write the test is so much slower, and distracts my focus from the code being written, and also when browsing the code it's so much easier to find related unittests nearby to see if a particular case has been tested or not, as opposed to having to open up another file and searching for tests that *might* be relevant.

Case in point: just this week, for a particular project of mine I needed to write external tests because refactoring would be too hard. So I cooked up a simple test driver that scans a set of directories to pick up test inputs and expected outputs. It was great that in D, this could be done in just a couple of days' worth of work.  However, I did find that needing to open up another file to create a new test, a second file to put the expected output, and a third file to look at the source code being tested, was a big slowdown for me.  I have to stop to think about which subdirectory under test/ I should put the relevant file(s), or if there's already a test there I have to stop to think about which filename I saved it under, etc..  It's just yet another mental hurdle to jump over while my already-busy brain is thinking about the code itself. In my case it's a necessary evil, but would I do it for a project that could just use inline unittests?  Definitely not.  But of course, YMMV.

2) Compilation times: perhaps if your unittests are too heavy, compilation times could become a problem, but IME, they haven't been. Plus, my opinion is that when you're compiling the code, you *should* be running unittests anyway (otherwise regressions inevitably slip in), so you're going to have to pay for the time taken to compile them regardless. In that sense, it's actually better to have them in the same file so that the compiler doesn't have to open up another file and allocate resources for handling another module.

3) version(unittest): it's true that this can be a problem.  I remember that in Phobos we used to merge PRs containing code that compiles fine with -unittest, but in real-world code doesn't compile because it has stuff outside unittests that depend on imports/declarations inside version(unittest).  This is actually one of the reasons I was (and still am) a big supporter of local/scoped imports. It may be more convenient to just put global imports at the top of the module, but it just creates too many implicit dependencies from mostly-unrelated chunks of code, that I'm inclined actually to call global imports an anti-pattern.  In fact, I'd even go so far to say that version(unittest) in general is a bad idea.  It is better to factor out stuff inside version(unittest) to a different module dedicated to unittest-specific stuff, and have each unittest that needs it import that module.  This way you avoid polluting the non-unittest namespace with unittest-specific symbols.

As for the dub-specific problems introduced by version(unittest): IMO that's a flaw in dub.  I should not need to contort my code just to accomodate some flaw in dub.  Having said that, though, I do agree that version(unittest) itself is a bad idea, so code written the way I recommend would not have this problem even given dub's flaws.


T

-- 
Programming is not just an act of telling a computer what to do: it is also an act of telling other programmers what you wished the computer to do. Both are important, and the latter deserves care. -- Andrew Morton

On Thursday, 22 March 2018 at 16:30:37 UTC, H. S. Teoh wrote:
> As for the dub-specific problems introduced by version(unittest): IMO that's a flaw in dub.  I should not need to contort my code just to accomodate some flaw in dub.  Having said that, though, I do agree that version(unittest) itself is a bad idea, so code written the way I recommend would not have this problem even given dub's flaws.

There's no "dub-specific problems". Article is wrong about that: when you run `dub test` only package you are working with is compiled with '-unittest' option. This way it is _impossible_ to have any kind of problems with `version(unittest)` if you are writing libraries

On Thursday, March 22, 2018 09:30:37 H. S. Teoh via Digitalmars-d-announce wrote:
> 2) Compilation times: perhaps if your unittests are too heavy, compilation times could become a problem, but IME, they haven't been.

Personally, I agree with you, but Atila is one of those folks who gets annoyed when tests take even a fraction of a second longer, so stuff that many of us would not consider pain points at all tends to annoy him. I prefer it when tests run fast, but if they take 1 second rather than 500 milliseconds, I don't consider it a big deal. Atila would.

So, given Atila's preferences, it makes sense to remove the tests from the module if that speeds up compilation even a little, whereas personally, I'd much rather have them next to the code they're testing so that I can see what's being tested, and I don't have to go track down another file and edit the tests there when I'm editing the code being tested. I'd prefer that that not harm compilation time, but if it does, I'm generally going to put up with it rather than move the tests elsewhere.

> 3) version(unittest): it's true that this can be a problem.  I remember that in Phobos we used to merge PRs containing code that compiles fine with -unittest, but in real-world code doesn't compile because it has stuff outside unittests that depend on imports/declarations inside version(unittest).  This is actually one of the reasons I was (and still am) a big supporter of local/scoped imports. It may be more convenient to just put global imports at the top of the module, but it just creates too many implicit dependencies from mostly-unrelated chunks of code, that I'm inclined actually to call global imports an anti-pattern.  In fact, I'd even go so far to say that version(unittest) in general is a bad idea.  It is better to factor out stuff inside version(unittest) to a different module dedicated to unittest-specific stuff, and have each unittest that needs it import that module.  This way you avoid polluting the non-unittest namespace with unittest-specific symbols.

I don't think that global imports are really an anti-pattern, though there are advantages to local imports. The big problem with global imports is when they're versioned, because then it's way too easy to screw them up. If they're not versioned, then worst case, you're importing something which wouldn't necessarily have to be imported if local imports were used.

> As for the dub-specific problems introduced by version(unittest): IMO that's a flaw in dub.  I should not need to contort my code just to accomodate some flaw in dub.  Having said that, though, I do agree that version(unittest) itself is a bad idea, so code written the way I recommend would not have this problem even given dub's flaws.

dub makes the problem worse, but it's inherent in how D currently works. When I compile my project with -unittest, and I'm importing your library, your version(unittest) code gets compiled in. I'm not sure exactly what happens with the unittest blocks, since the code for them isn't generated unless the module is explicitly compiled, but I think that it's the case that all of the semantic analysis still gets done for them, in which case, anything they import would need to be available, and that's a huge negative. We really need a DIP to sort this out.

Now, unfortunately, dub makes the whole thing worse by compiling dependencies with their unittest target when you build your project with its unittest target, so you get all of the unit tests of all of your dependencies regardless of what dmd does, but even if dub were not doing that, we'd still have a problem with the language itself.

- Jonathan M Davis

On Thursday, March 22, 2018 16:54:18 Anton Fediushin via Digitalmars-d- announce wrote:
> On Thursday, 22 March 2018 at 16:30:37 UTC, H. S. Teoh wrote:
> > As for the dub-specific problems introduced by version(unittest): IMO that's a flaw in dub.  I should not need to contort my code just to accomodate some flaw in dub.  Having said that, though, I do agree that version(unittest) itself is a bad idea, so code written the way I recommend would not have this problem even given dub's flaws.
>
> There's no "dub-specific problems". Article is wrong about that: when you run `dub test` only package you are working with is compiled with '-unittest' option. This way it is _impossible_ to have any kind of problems with `version(unittest)` if you are writing libraries

I could be wrong, but I am _quite_ sure that dub builds all dependencies with their test targets when you build your project with its test target. I was forced to work around that with dxml by adding a version identifier specifically for dxml's tests _and_ create a test target specifically for dxml. Simply adding a dxml-specific version identifier to its test target was not enough, because any project which depended on dxml ended up with the dxml-specific version identifier defined when its tests were built, because dub was building dxml's test target and not its debug or release target. The only way I found around the problem was to create a target specific to dxml for building its unit tests and define the version identifier for that target only.

I had to add this to dub.json

    "buildTypes":
    {
        "doTests":
        {
            "buildOptions": ["unittests", "debugMode", "debugInfo"],
            "versions": ["dxmlTests"]
        },
        "doCov":
        {
            "buildOptions": ["unittests", "debugMode", "debugInfo",
                             "coverage"],
            "versions": ["dxmlTests"]
        }
    }

And then I have scripts such as

test.sh
=======
#!/bin/sh

dub test --build=doTests
=======

to run the tests. I had to actively work around dub and what it does with unit tests in order to not have all of dxml's tests compiled into any project which had dxml as a dependency.

- Jonathan M Davis

I understand your opinion and I think it is all reasonable. You talk about longer compile times since every D module is like a C++ header. That touches one of my pet peeves with the language or eco system as it stands and I wonder if you would agree with me on the following:

Libraries should be tied into applications using interface
files (*.di) that are auto-generated by the compiler for the
_library author_ with inferred function attributes. If after
a code change, a declaration in the *.di file changes, the
library's interface changed and a new minor version must be
released. The language must allow to explicitly declare a
function or method as @gc, impure, etc. so the auto-inferred
attributes don't later become an issue when the implementation
changes from e.g. a pure to an impure one.
Opaque struct pointers as seen in most C APIs should also be
considered for *.di files to reduce the number of imports for
member fields.

That means:
* No more fuzzyness about whether a library function
  will remain @nogc, @safe, etc. in the next update.
* Explicit library boundaries that don't recursively import the
  world.

-- 
Marco

On Thursday, 22 March 2018 at 13:58:50 UTC, Atila Neves wrote:
> On Thursday, 22 March 2018 at 12:26:14 UTC, Anton Fediushin
>> Tests in their own file is something from 90-s. It's 2018 and I want to be able to write tests anywhere I want.
>
> You _can_ write them wherever you want. I'm not arguing for taking any flexibility away, I'm arguing that for most projects it's a bad idea, and I stated the reasons why I think that's the case. Feel free to disagree.

Since your article is wrong about dub's way of handling `unittest` configuration (take a look at my last post), the only valid point is "they increase build times".

The only choice you have to make is: you take the red pill and choose convenience over compilation speed or you take the blue pill for speed over the convenience.
None of the choices is "good" or "bad". They're just slightly different.

>> "They increase build times" - fix compiler, not my code.
>
> If by "fix the compiler" you mean "make it so that the compiler knows exactly what files need to be recompiled if I only edited the body of a unittest block" (which is what would be needed), then I don't think that'll happen any time soon.
>
> But sure, if the compiler worked that way I'd happily advocate for writing unittests in the same file. But for now, I'd rather spend a lot less time staring at the screen for 2s waiting for my code to be built. That's my trade-off, and reasonable people may (and apparently quite obviously do) disagree.

I'm not a compiler developer, so I don't mean anything in particular. Just saying.
I didn't knew that change in unittest block causes rebuilding of many modules, so that's good to know.

>> "version(unittest) will cause you problems if you write libraries" - fix dub, not my code.
>
> This is _not_ a dub issue. version(unittest) _will_ bite you as soon as you compile foo.d with -unittest because its imports will be parsed differently. This is one to stick in the "fix the compiler" column. See the link to the since-reverted Phobos PR in the blog.

Yeah, I was wrong about dub. I'm so sorry since it turned out that dub handles `dub test` in the way that never causes problems with `version(unittest)`.

What about `version(unittest)` itself - as any of the conditional compilation features it should be handled with care. Especially when we are talking about cases where dub isn't used.

On Thursday, 22 March 2018 at 17:08:18 UTC, Jonathan M Davis wrote:
> I could be wrong, but I am _quite_ sure that dub builds all dependencies with their test targets when you build your project with its test target.

I thought so too, but I just checked and it doesn't do that. I'd better create a test repository for that, maybe I'm doing something wrong.

> I had to add this to dub.json
>
>     "buildTypes":
>     {
>         "doTests":
>         {
>             "buildOptions": ["unittests", "debugMode", "debugInfo"],
>             "versions": ["dxmlTests"]
>         },
>         "doCov":
>         {
>             "buildOptions": ["unittests", "debugMode", "debugInfo",
>                              "coverage"],
>             "versions": ["dxmlTests"]
>         }
>     }

Well, that's just ugly. And that `versions` thing looks exactly like the one from Atila's article.

So? Am I wrong about dub? Let me investigate

On Thursday, 22 March 2018 at 16:30:37 UTC, H. S. Teoh wrote:
> On Thu, Mar 22, 2018 at 10:59:56AM +0000, Atila Neves via Digitalmars-d-announce wrote:
>> Blog post:
>> 
>> https://atilanevesoncode.wordpress.com/
> [...]
>
> I realize this is your opinion, but I disagree with them because:

Disagreeing is more than fine. :)

> 1) I've found that having unittests built into the language is a big win, *especially* because I can write tests next to the code being tested.

I'm confused. I don't know what I wrote that made you think I believe otherwise. I wrote "fantastically successful for the language" and that "Let me start by saying that some tests should go along with the production code".

> being tested, was a big slowdown for me.  I have to stop to think about which subdirectory under test/ I should put the relevant file(s), or if there's already a test there I have to stop to think about which filename I saved it under, etc..  It's just yet another mental hurdle to jump over while my already-busy brain is thinking about the code itself.

Then I'd recommend:

1) To write the tests inline when the mental burden is high
2) Move them afterwards when one's brain can think of where to place them.

I think that mirroring the production source tree is probably the way to go most of the time, i.e. test/foo/bar/baz.d for src/foo/bar/baz.d.

> 2) Compilation times: perhaps if your unittests are too heavy, compilation times could become a problem,

Maybe I wasn't clear in what I wrote: I'm not saying that I notice the increase in compilation times of the tests themselves. I probably haven't but I'd have to measure to know for sure. I'm saying that if you change anything in a D module, it must be assumed that any other module that imports the module you just edited, even if transitively, must be recompiled. So if I edit a test in D, under normal circumstances, I _have_ to recompile several files. It's not the unittest itself that is heavy, it's recompiling everyone who depends on the module that said unittest happens to find itself in.

> but IME, they haven't been.

IME most other people find it bizarre I get angry at 2s incremental rebuild times. Anything over 100ms is an eternity.

> Plus, my opinion is that when you're compiling the code, you *should* be running unittests anyway (otherwise regressions inevitably slip in),

Yes.

> so you're going to have to pay for the time taken to compile them regardless.

Yes.

> In that sense, it's actually better to have them in the same file so that the
> compiler doesn't have to open up another file and allocate resources for handling another module.

Noooooooooo.

> As for the dub-specific problems introduced by version(unittest): IMO that's a flaw in dub.

It's not dub-specific at all. It's same problem that you reference in:

> I remember that in Phobos we used to merge PRs containing code
> that compiles fine with -unittest, but in real-world code doesn't compile because it has stuff outside unittests that depend on imports/declarations inside version(unittest).

I used dub as an example. Anyone else would have the same problems if they hand-wrote Makefiles using git submodules as dependencies.

Atila

On Thursday, 22 March 2018 at 16:54:18 UTC, Anton Fediushin wrote:
> On Thursday, 22 March 2018 at 16:30:37 UTC, H. S. Teoh wrote:
>> As for the dub-specific problems introduced by version(unittest): IMO that's a flaw in dub.  I should not need to contort my code just to accomodate some flaw in dub.  Having said that, though, I do agree that version(unittest) itself is a bad idea, so code written the way I recommend would not have this problem even given dub's flaws.
>
> There's no "dub-specific problems". Article is wrong about that: when you run `dub test` only package you are working with is compiled with '-unittest' option. This way it is _impossible_ to have any kind of problems with `version(unittest)` if you are writing libraries

IMHO dub does it exactly right - I most definitely do _not_ want to build my dependencies's unittests, nor do I want to run them.

Imagine if I had to parse and run the tests from libclang or Qt (worse: both!) every time I wrote a C++ program...

Atila

On Thursday, 22 March 2018 at 17:15:55 UTC, Anton Fediushin wrote:
> So? Am I wrong about dub? Let me investigate

I'm not wrong! It works as expected: only package you are working with compiles with `-unittest` option.

Test repo: https://github.com/ohdatboi/dub-please-be-as-cool-as-i-think-you-are

Cd to `app` directory and run:
---
$ dub
Performing "debug" build using /usr/bin/dmd for x86_64.
lib1 ~master: building configuration "library"...
lib1: Compiled without -unittest option
app ~master: building configuration "application"...
app: Compiled without -unittest option
Linking...
Running ./app
---

---
$ dub -b unittest
Performing "unittest" build using /usr/bin/dmd for x86_64.
lib1 ~master: building configuration "library"...
lib1: Compiled without -unittest option
app ~master: building configuration "application"...
app: Compiled with -unittest option
Linking...
Running ./app
---

Oh heck yeah! I think that dub is only one of the D tools which never disappoints me.