Remove version(unittest) imports to avoid extra imports in user code.

[schveiguy]

Another batch, see #6450 for details.

[dlang-bot]

Thanks for your pull request, @schveiguy!

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 + phobos#6451"

[schveiguy]

Since in-function structs depend on the order defined, these have to be defined at module-level. Did not work if I put it inside the unittest itself.

But these structs are super-simple, and have no external dependencies.

[schveiguy]

Not a fan of unittests being inside version(unittest), that's why the indentation changed.

[schveiguy]

Note, these were copied as-is to this unittest. I'm not in love with the implementation, but wanted to keep the PR simple.

[andralex]

This movaroo wasn't quite needed because it doesn't import anything. But fine by me.

[schveiguy]

ping @marler8997

[marler8997]

Haven't reviewed in detail but changes look fine at the surface level.

[marler8997]

Since this decision is going to affect a fair amount of code in ALL of phobos. Would like to get an opinion from @andralex whether this is the direction we want to go in.

From what I can see, what were'e doing is taking all the unittests that have shared code and merging them into one "super unittest". Then we move the shared code that use to be inside version(unittest) and move that inside the "super unittest" as well. If there are cyclic dependencies between structures in this "shared code", then they are pulled out and put inside version(unittest) since types inside functions don't support forward referencing. The hope is that there are no types like this that are going to reference external types causing more modules to be loaded (so having to compile them won't have a noticeable affect on compile-time).

Besides the forward reference problem, the other problem I see is that this forces all the unittests using shared code to be in the same block. This may be less than ideal if a unittest is for a specific function and now it has to be moved away from the function it is testing. This also won't work if it is a "documentation" unittest.

[schveiguy]

what were'e doing is taking all the unittests that have shared code and merging them into one "super unittest".

To clarify, this is only happening inside std.concurrency. And really, the "super unittest" consists of 2 very similar small adjacent unittests, which were trivial to put together.

If there are cyclic dependencies between structures in this "shared code", then they are pulled out and put inside version(unittest)

This is regarding the structs in std.conv? No, those were already in a version(unittest). I just left them there because they had to be, and moved the static assert into the accompanying (normal) unittest. But they are really simple. If they were more complex (i.e. imported more files), I would have had to rework the code to accomplish the task.

this forces all the unittests using shared code to be in the same block.

The qualifier is that version(unittest) code should not import other modules or instantiate significant templates. In some cases (e.g. std.encoding) I took the liberty of moving code that is used inside only one unittest into that unittest, even though that code was not in this vein. IMO, the more stuff we can move inside actual unittests, the better, since it won't even be parsed when your PR is merged.

There is no requirement for all unittests to be in the same block, nor is that required really anywhere, even in the std.concurrency example. I could have potentially left those two functions out (and moved the imports inside them), but merging the only 2 unittests that utilize the functions seemed simpler, and not a drastic change.

Just noticed, the std.concurrency unittest imports std.stdio, but doesn't actually use it. Will remove.

[schveiguy]

A further note: I would like to remove as many version(unittest) blocks as possible (for the sake of grepping for this problem), and moving single-use functions/types inside actual unittests furthers this goal.

[marler8997]

To clarify, this is only happening inside std.concurrency. And really, the "super unittest" consists of 2 very similar small adjacent unittests, which were trivial to put together.

I'm just trying to summarize our strategy for this. Do we have other strategies for solving the "shared unittest code" problem, or are we planning on using this strategy throughout? I know that we may not know what's needed until more modules are looked at, but I want to make sure the strategy you've used here is what we want.

This is regarding the structs in std.conv? No, those were already in a version(unittest).

Right. I was just saying that you "would have put them" inside the unittest block (following the strategy I outlined) but couldn't in this case because of the forward reference issues, so you left them in the version(unittest) block. This is an example of a weakness which we should consider when evaluating what strategy to take.

There is no requirement for all unittests to be in the same block, nor is that required really anywhere, even in the std.concurrency example.

Right, I'm just saying that if there is "shared code" between unittests, this seems to be the strategy you've chosen. Since the code inside the version(unittest) block in this case wasn't "shared", you were able to simply move it inside the unittest block.

[andralex]

yah imports in version(unittest) are at least with the current semantics a latent source of bugs

[andralex]

This movaroo wasn't quite needed because it doesn't import anything. But fine by me.

[andralex]

I would like to remove as many version(unittest) blocks as possible (for the sake of grepping for this problem)

That sounds a bit extreme, unittest fixtures are a common idiom. Let's not take this too far, thanks.

[andralex]

sry wrong button

[schveiguy]

Do we have other strategies for solving the "shared unittest code" problem, or are we planning on using this strategy throughout?

I plan on moving all code that I can inside unittests. A goal of mine is to eliminate as many instances of version(unittest) as possible. It's like @trusted blocks -- they are very hard to review to see all the cases that may occur.

For example, if a version(unittest) function returns a template instantiation, even though there are no imports directly in the block, the instantiation may import other things, and you'd have to go review the template to see. Much easier to just stick it in a unittest which you know isn't even going to be semantic'd (eventually).

There are other possibilities as we have discussed elsewhere -- i.e. making everything templates so they aren't actually semantic'd until used from a unittest.

I know that we may not know what's needed until more modules are looked at

I've looked at them all, and there are a few crappy cases. I'm saving them for last, these are the easy ones 😉

I'm fine with shared code in version(unittest) blocks, if that's what makes the most sense. But consider something that is really important -- this is the base library that ALL code uses. We can probably be more strict about what we allow in version(unittest) blocks than most other code bases. We want phobos unittests to have 0 impact if you are testing your code, period.

To that end, what may be the best solution is to put such shared items inside std.internal, and import those inside the unittests. But I don't know that we need to go that far (yet). This exercise is a basic cleanup of the easy things.

[marler8997]

Nice verb..."semantic'd" :)

I think we need to treat version(unittest) differently than we have been. We should treat it as a way to provide "unittest support/framework" code for other modules. This isn't really a common use case so I would expect it to be used very rarely.

What we currently use them for is to define shared code between unittests within a module. I expect there to be cases in phobos where there is alot of shared usage of a common set of code. We could use your strategy to create a "super unittest" to solve this problem but that does have some disadvantages (moving tests around/forward reference problems). I'm just trying to see if theres a better way to solve this, but I guess we'll see when we get to those problem modules.

[schveiguy]

I think we need to treat version(unittest) differently than we have been. We should treat it as a way to provide "unittest support/framework" code for other modules.

I like the idea. Just unittest/support in general (for current and other modules).

[quickfur]

IMO, if there is common code shared between unittests, said common code should be factored out into its own module (e.g., std.internal.tests.*) and imported from inside the relevant unittest blocks.

version(unittest) is one of those things that look clever and neat at first glance, but with more usage you come to realize that it's a minefield of problems and latent bugs. I would avoid using it unless there is absolutely no other alternative.

[marler8997]

IMO, if there is common code shared between unittests, said common code should be factored out into its own module (e.g., std.internal.tests.*) and imported from inside the relevant unittest blocks.

Yes this is another strategy that would work. Thanks for the idea. @schveiguy Maybe this is the solution we could use for the more "problemantic" modules?

[marler8997]

@quickfur

I'm curious if you could mention any problems you know of with version(unittest). The problems that I can think of would be alleviated if we could solve the problem I talked about here (https://forum.dlang.org/post/nbdzcwlgzvdcxnecefdk@forum.dlang.org). Are there other problems with it that this wouldn't solve (besides the performance problems)?

[schveiguy]

Yep, I like the idea (see my comment, "what may be the best solution is to put such shared items inside std.internal" ), but it may be overkill for simple things. There may not be a hard set of rules we can come up with that fixes all situations, and I'm not sure we are ready to define it yet in any case.

[quickfur]

version(unittest) lets you insert module-level declarations depending on whether you're compiling with -unittest. In the past, this led to bugs in Phobos where some non-unittest code was depending on declarations pulled in inside version(unittest), but since the autotester always builds with -unittest, such bugs are not detected until the PRs introducing these hidden dependencies have already been merged. But when user code uses the code and compiles without -unittest, you get bugs, But these bugs disappear when you compile with -unittest. Very annoying to track down and fix. Basically, it boils down to the library author testing one version of the code while shipping a different version, which reduces the value of unittesting (you're not actually testing what you're shipping).

Using version(unittest) also doesn't work well with ddoc'd unittests, because the code example would compile, but contain declarations nowhere visible in the docs, which is very confusing to users reading the docs. (This same argument could also be used against global imports, which is why I support moving top-level imports into scoped imports instead, even though you could argue it introduces boilerplate.)

[marler8997]

Thanks for the explanation @quickfur . That's basically what I figured. Though I hadn't thought of the confusing ddoc case.