Move core.time non-documentation unittests to tests/time

[n8sh]

Tests like these that run in CI don't need to be repeated in every user's unittest builds. This can be done for other tests too. I am dividing the work among several PRs to make the review easier.

[dlang-bot]

Thanks for your pull request, @n8sh!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2316"

[jmdavis]

Why do you think that all of the unittest blocks need to be moved out of core.time in order to have them not be compiled into the code of folks importing core.time? Moving the tests like this is a serious problem for maintainability, and it should be completely unnecessary. If there is a language-level problem that needs to be fixed here, then we need to fix that, not move all of the tests.

[jmdavis]

And that goes for druntime and Phobos in general. Putting the tests in a separate place from the code makes it much harder to maintain the tests and keep track of what's being tested. Being able to put the unit tests right after the functions is a key feature of unit tests in D. If we can't do that, we have a serious problem, and it needs to be fixed. AFAIK, outside of templates, the only problem with unittest blocks right now is that they get evaluated when you import a module when compiling your modules for your unit tests - which shouldn't be happening - but unless I'm mistaken, the tests themselves are not actually compiled in, so they're not being run. We do need to look at doing something like treating unittest blocks as version(none) when a module is imported, but that shouldn't necessitate moving unittest blocks. Is there some other problem that you're trying to fix with these changes?

[n8sh]

Moving the tests like this is a serious problem for maintainability

Please explain your concerns.

Why do you think that [non-documentation] unittest blocks need to be moved out of core.time

Aside from the reasons in my first comment, it makes the source code of core.time more readable. Those tests were 1920 lines of a 4773 line file. Moving them means there is no need to compromise readability for thoroughness or vice versa.

[jmdavis]

Please explain your concerns.

I would have though this was obvious and that I stated it clearly enough. The fact that unittest blocks can go immediately after what they test is a key feature in D and has been talked about as such plenty of times in the past. It makes it easy to see what tests exist for a function. It makes it easy to move between the function and the tests for that function when working on the code. Having everything in one place just makes it all around easy to keep track of it all and maintain it. If tests are in a separate file, it makes it a lot harder to know which functions have been properly tested, and you have to start hopping around to work on things. I thought that that was something that was generally accepted around here.

As an example, the Interval types in std.datetime are templates, so I originally put the tests for them after the struct definitions in order to avoid having them be included in every template instantiation, and that has made it a royal pain to work on them - and they're in the same file. I've slowly been working on fixing them to do more like what Steven did with RedBlackTree and make a single instantiation include the tests so that they can be inside the template, but having them outside has made maintaining them far harder, and I really, really, really don't want to see us move to having the tests in separate files. There shouldn't be a need for it. And certainly, if we do do it, we'd better have an extremely good reason for it with a general consensus about it. If we're doing it because of concerns about unittest blocks being processed by the compiler when the module is imported, then that affects the entire language, not just druntime and Phobos, and we should fix that in the language, not start moving tests around just to try to work around a problem in the language - especially when it makes maintaining the code harder.

Aside from the reasons in my first comment, it makes the source code of core.time more readable. Those tests were 1920 lines of a 4773 line file. Moving them means there is no need to compromise readability for thoroughness or vice versa.

I really don't think that the way that it's been has been a problem. It's the way that druntime and Phobos in general have been, and certainly for me personally, it's worked great. The tests interspersed with the code makes them easy to find, and my experience is that it makes the code far easier to maintain. And it should be trivial to skip past unittest blocks in your code editor if that's what you need to do. Moving the unittest blocks into separate files is a drastic departure from what we've been doing for years and should have a really good reason behind it for even considering it.

[jacob-carlborg]

And it should be trivial to skip past unittest blocks in your code editor if that's what you need to do

It should be trivial to switch back and forth between the implementation file and the unit test file as well in your editor (assuming a convention is followed).

Having the unit tests separate works tremendously well in the Ruby world. I have never seen so many projects unit tested and so well covered as Ruby projects.

[CyberShadow]

Unit tests in D should be near their tested item's implementation.

The stand-alone tests are needed only for cases when a unit test is not applicable for some or other reason.

[jmdavis]

It should be trivial to switch back and forth between the implementation file and the unit test file as well in your editor (assuming a convention is followed).

I've dealt with both, and my experience is that it works far better to have the tests right next to the functions that they're testing. D makes that work really well, and it's what we've generally promoted. If we change it in druntime or Phobos now, we need a really good reason to do so. And if it's because of a problem with the language (e.g. unittest blocks are still processed when the module is imported), then we should be fixing the language so that everyone benefits.

[n8sh]

AFAIK, outside of templates, the only problem with unittest blocks right now is that they get evaluated when you import a module when compiling your modules for your unit tests - which shouldn't be happening - but unless I'm mistaken, the tests themselves are not actually compiled in, so they're not being run.

I hadn't realized that. That removes nearly all of the reason for this PR.

[CyberShadow]

Oh, I had misunderstood the PR description.

Tests like these that run in CI don't need to be repeated in every user's unittest builds.

I thought "user" here means "Phobos contributor" (as in, they should run the Phobos tests on their machine but not the core.time tests for some reason).

[jmdavis]

I hadn't realized that. That removes nearly all of the reason for this PR.

We really should look into making it so that unittest blocks are treated as version(none) when importing, and if that can be done cleanly from an implementation perspective, I don't expect that there would be a big problem getting it accepted. However, fixing the issue about unittest blocks in templates being compiled into every instantiation of a template unless they're versioned out with a static if or a version identifier is a thornier issue. For now at least, the solution that seems to be accepted is to use static ifs on all of the unittest blocks inside a template and make it so that the condition is true for only one instantiation of the template (preferably for a dummy instantiation that's only used for testing but if not, then only one real one). I'd love to do better, but the last time it came up, Andrei wasn't open to adding a language feature like an attribute to mark unittest blocks inside a template as being only syntactically in the template. He pretty much outright told me that I'd be wasting my time if I wrote a DIP on the subject.

The last remaining issue that I'm aware of is when you need a version(unittest) symbol, since you don't really want those to be imported in most cases. However, it is desirable to be able to put version(unittest) symbols in modules to reuse them for testing (e.g. ranges specifically designed for use in unit tests only in your project). So, it really doesn't make sense to make it so that you can't import version(unittest) symbols (not to mention, such a change would definitely break code), and in general, it's actually risky to put version(unittest) inside a normal module anyway, since it's not necessarily hard to accidentally mark something with it that you didn't mean to. So, if it's just best practice to put version(unittest) symbols in their own modules and only import them inside of unittest blocks, they're really not a problem anymore - at least as long as unittest blocks are treated as version(none) when importing.

I've been considering writing a DIP on treating unittest blocks as version(none) when importing (the fact that they aren't actually resulted in linker errors with dxml's tests due to issues with version(unittest) symbols which were just imported in unittest blocks), but I need to find the time to get it all sorted out and make sure that I have all of the facts right first. I think that I do, but I obviously have to get them right in the DIP, or it's not going to go very well.

[n8sh]

However, fixing the issue about unittest blocks in templates being compiled into every instantiation of a template unless they're versioned out with a static if or a version identifier is a thornier issue.

We might not even want to "fix" this, since if the test actually varies based on the template parameters we want it to run. phobos/6693 has an example of a templated unittest that could catch a real error:

@system unittest
{
    assert(typeof(this).init.isNull, typeof(this).stringof ~
        ".isNull does not work correctly because " ~ T.stringof ~
        " has an == operator that is non-reflexive and could not be" ~
        " determined before runtime to be non-reflexive!");
}

[jmdavis]

We might not even want to "fix" this, since if the test actually varies based on the template parameters we want it to run. phobos/6693 has an example of a templated unittest that could catch a real error:

Really, the ideal solution is having the choice. Sometimes, a test is generic enough that it works to have it compiled in with every instantiation, and you get better test coverage that way, since it ensures that it works correctly (at least with regards to that test) for every type that it's instantiated with. However, most tests can't be generic like that, and so for most tests, it just means that you have exactly the problem that you were worried about for this PR - only worse. The tests end up in the user's code once for every instantiation they use (not just once for having imported the module) - and get run with their unit tests. So, ideally, there would be a way to indicate whether a test was supposed to be instantiated with the template or whether it was just inside the template for convenience (be it for ease of maintenance or so that it can be a ddoc-ed unittest).

I wrote a DIP for the old DIP process that suggested that if we marked unittest blocks inside templates with static that they would then only be inside the template syntactically and would be treated just like if they were outside the template. That way, they would not to be instantiated with the template, and you wouldn't have to include an instantiation of the template outside of the template just to ensure that the tests got run. But any unittest blocks not marked with static would still function as they do now, so the decision of how each unittest block would be treated would be left up to the programmer writing the template. Walter and Andrei never really reviewed the DIP, but Timon objected big time to the use of static; I thought that it made a lot of sense (i.e. it went with the template as a whole rather than a specific instantiation of the template just like a static member of a struct goes with the struct as a whole rather than a particular instance), but he had definition for static that somehow fit consistently with every use of static that we currently have (I'm not sure how, and I don't remember it) but did not fit consistently with my proposed usage. I'd be fine with creating a different attribute for it instead, but at some point when it was being discussed towards the beginning of the new DIP process, and I said something about creating a DIP with the new process, Andrei pretty much outright said that it would be a waste of my time, because he considered the current workarounds to be perfectly acceptable. I don't agree, but it's not my decision.

[schveiguy]

Let me interject some things I learned when I was doing some experimentation for a DMD PR that was supposed to remove unittests when those tests were instantiated inside imported modules (that PR was later abandoned, but I wish we had worked through the issues).

1. unittests that are not part of templates are parsed when imported, they are not semantically analyzed. This means they play no significant role in imported modules, and should be perfectly fine to include inside modules alongside the implementations they support (parsing is very quick and relatively low cost).
2. The real problem is that when unittests are enabled, a flag is set to compile in all instantiations of templates, even when those instantiations occur inside imported modules. There is a special dmd flag for this called -allinst which activates the mechanism orthogonally, but no way to turn it off.

The second point means that extra imports get added (ones inside locally instantiated templates), and those extra imports cause more templates to be instantiated, and included, etc. In addition, any unittests inside of those templates are instantiated locally, as if they were written in the importing module (which is really dumb in a way -- you likely are running the same unittests inside every importing module).

But module-level unittests are not the problem there, even templates instantiated inside an imported unittest are not semantic'd, so the instantiation doesn't happen, even with the -allinst flag on.

IMO, we should not concentrate at all on removing unittests from modules. We should concentrate on fixing the limitations of DMD that caused the -allinst flag to be left on for unittests, and as much as possible, remove unittests from templates. Yes, I put unittests inside RedBlackTree that I thought were clever enough to avoid having them instantiated when they shouldn't be, but I don't think it prevents user code from instantiating them. I very much fully support the static unittest idea that Jonathan has.