Skip to content

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Sep 29, 2011

Change the way to run unit testings in Windows like in Posix.

In Posix, unit testing is done for each module, but in Windows, unittest.exe that contains all the unit testings is generated and runs.

But the way for Windows is bad, because it introduce unnecessary dependencies for unit testings.
Example: std.conv is imported in std.algorithm for using to template function, but to is only used in unittest blocks, not in algorithm functions. The dependency is only necessary for unit testing, not need for main features of std.algorithm.

We should run unit testing like Posix, but in Windows DigitalMars make is less usable than GNU make in Posix. Then I wrote simple utility for the purpose (I don't like request cygwin-make for Phobos unit testing).

eachtest.d is only dependent to druntime, not to Phobos`, and doesn't need strange cycle dependencies.

I think this isn't the best, but is the better way.

@jmdavis
Copy link
Member

jmdavis commented Sep 30, 2011

Oddly enough, these changes make the unit tests run slower on my machine. That doesn't mean that we shouldn't make the changes, but it's odd, since the changes should have done the opposite.

It be nice if the unit tests actually ran in order though. The current order seems quite random.

@9rnsr
Copy link
Contributor Author

9rnsr commented Sep 30, 2011

Oddly enough, these changes make the unit tests run slower on my machine.

Slower testing is the same with my machine (64-bit Windows 7, Core i7 2GHz, 4GB memory), it is about +1 minute.
I think it is the appearance of I/O overhead, that is separating compile and linking, and/or each exe loading and running.
Then, it is necessary overhead.

It be nice if the unit tests actually ran in order though. The current order seems quite random.

Fixing it is easy, arrange files of $(SRCS) in win32.mak.

@ghost
Copy link

ghost commented Sep 30, 2011

old: Elapsed Time: 0:03:48.390
new: Elapsed Time: 0:03:25.578

They're faster on my machine. It's just a regular 7200RPM HDD with a quad-core AMD @2.6ghz.

@ghost
Copy link

ghost commented Sep 30, 2011

This runs in 1 minute 58 seconds for me:

import std.path;
import std.file;
import std.process;
import std.parallelism;

void main()
{
    string[] files;
    foreach (string entry; dirEntries(rel2abs(join(curdir, "std")), SpanMode.depth))
    {
        if (entry.isFile && entry.getExt == "d")
        {
            files ~= entry;
        }
    }

    foreach (file; taskPool.parallel(files, 1))
    {
        system("rdmd --main -d -unittest " ~ file);
    }
}

Can't tell if that's a good way to unittest though. (it should of course skip osx/linux files).

@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 1, 2011

Added parallel testing by using std.parallelism.
Score:
Parallel ... 3m26s
Sequential ... 7m11s

I think it is better that use the parallel version for default testing, and use the sequential version when std.parallelism is broken.

@jmdavis
Copy link
Member

jmdavis commented Oct 1, 2011

I'm not sure that Phobos can actually be safely tested in parallel. I know that it was brought up not to long ago, and several things were pointed out which would need to be changed. For instance, std.file creates several files in the process of its tests, and if any other tests created any of those files, then you have race conditions which will affect the tests. It's definitely true that the unit tests within a module cannot be run in parallel, but that's a separate issue entirely, and the unit test framework isn't setup to do that at the moment even if we wanted it to (which is questionable).

Regardless, I'm not sure that it's safe to run the unit tests in parallel, and we need to verify that it's safe before we do it.

@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 1, 2011

Researched roughly with temporary file name used in each unit testing.

std.base64
    ./testingEncoder
    ./testingDecoder
std.file
    ./unittest_write.tmp
    ./unittest_write2.tmp
    /tmp/deleteme.dmd.unittest
    /tmp/deleteme.dmd.unittest_slink    (Posix only)
    /tmp/deleteme.dmd.unittest_file
    /tmp/deleteme.dmd.unittest_dir      (linux only)
std.mmfile
    testing.txt (Memory mapped file)
std.regex
    /tmp/deleteme   (Today test is masked)
std.stdio
    ./deleteme
    ./testingByLine
    ./testingByChunk
    ./dmd-build-test.deleteme.txt
std.stream
    ./stream.$$$
std.cstream
    ./stream.txt
std.parallelism
    ./tempDelMe.txt

Fortunately, there is no conflict between modules, but the result is much dangerous.
I think we need a way to create tmpfile more safely, but std.stdio.File does not support "x" mode in Windows.

@WalterBright
Copy link
Member

I'd prefer to leave it as-is because it's also a great test for dmd to compile a large app all at once.

@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 1, 2011

Building the large unittest.exe at once is good test for dmd, but it is harmful for Phobos.
The unittest.exe contains all unit tests of modules, and it forces to avoid cycle dependencies between them.
It will limit the use other modules for the tests in a module.
Even worse, to avoid cycle dependencies in unittest.exe, it is forbidden to use other modules for a module feature (not unit testing). It is really problem.

Then, making unittest.exe is harmful for Pohbos. I think it is useless to leave it as-is.

@jmdavis
Copy link
Member

jmdavis commented Oct 2, 2011

Actually, I'd argue that we set it up so that the unit tests can be compiled both as one single executable and as separate executables on both Windows and Posix. Being able to catch circular dependencies and test dmd's efficiency with a single executable is very valuable, but we also want the unit tests to run quickly. So, having both is valuable. And honestly, if compiling the Windows unit tests takes longer when compiling separately (as currently appears to be the case), I'd argue that we should favor having the single executable on Windows rather than splitting them up. The primary gain from splitting them that I see is to cut down on the time it takes to compile and run them. And losing the ability to have Phobos find its own circular dependencies is definitely a problem, so having no unit test builds that build everything as one is defnitely a problem.

And as for circular dependencies, if those are a problem, then I think that the language should be improved to provide a way to resolve them. They're frequently a pain to find and problematic to fix. We've found workarounds thus far, but it's definitely one of the language's rough edges. So, if they're causing us more problems, we should find a way to fix the problem in general, not just work around it in Phobos' unit tests.

@9rnsr
Copy link
Contributor Author

9rnsr commented Oct 2, 2011

I think the dependencies for module features and for unit tests is definitely different.
The primary purpose separating the tests is removing the matter that latter dependencies limit former ones.

It is real problem. In pull #276, I've add a dependent from std.datetime to std.windows.registry, but it cause circular dependency error only in Windows unittest.exe. After separating tests, there is no error.

Speed up testing is not main purpose. I've added parallel testing for it, but I don't argue to change it default.

@jmdavis
Copy link
Member

jmdavis commented Jan 22, 2012

I don't see any reason to leave this open, given that Walter's against it, and it's obviously not being merged in at this point.

I think that if we want to make changes like this moving forward, we should make it possible to build both all of the unit tests at once and all separately on both Posix and Windows so that we can get the benefit of both tests. But I think that the default of having Posix build separately and Windows build together is good, given that it gets us the chance to test Phobos being both built separately and together.

As for the circular dependency between std.datetime and std.windows.registry, it's no longer an issue, since std.datetime no longer has any static constructors, and in general, it seems the consensus that we need to avoid static constructors in Phobos if we can, since they not only can cause circular dependencies, but they end up increasing the size of executables.

@jmdavis jmdavis closed this Jan 22, 2012
marler8997 pushed a commit to marler8997/phobos that referenced this pull request Nov 10, 2019
Merge remote-tracking branch 'upstream/stable' into merge_stable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants