Skip to content

Small improvements to unit_tester_runner + run.d #9348

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wilzbach wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Small improvements to unit_tester_runner + run.d #9348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
90ce29e
Document unit_tests + add synonyms
wilzbach Feb 10, 2019
c23f497
Add logging to the unit_test_runner
wilzbach Feb 10, 2019
61effc9
Use relativePath instead of a hard-coded replace
wilzbach Feb 10, 2019
306a5aa
Mark logging in run.d with [run.d]
wilzbach Feb 10, 2019
a31a60c
Use dmdSrcDir for projectDir.buildPath('src', 'dmd')
wilzbach Feb 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions test/run.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ Examples:
./run.d runnable/template2962.d # runs a specific tests
./run.d runnable/template2962.d fail_compilation/fail282.d # runs multiple specific tests
./run.d fail_compilation # runs all tests in fail_compilation
./run.d unit_tests # runs all unit tests
Copy link
Contributor

@jacob-carlborg jacob-carlborg Feb 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should not document it since the -u flag exists as well.

Copy link
Contributor Author

@wilzbach wilzbach Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but it's documented in the README.md:

The unit tests will automatically run when all tests are run using ./run.d or
make. To only run the unit tests the ./run.d unit_tests command can be used.
For a more finer grain control over the unit tests the ./run.d -u command can
be used:

I guess as long as we're consistent I'm fine either way. Other thoughts.

Btw currently there's no easy way in to run just the integration tests (i.e. compilable, runnable, fail_compilation). Maybe it's worth adding tests as a shortcut?

Copy link
Contributor

@jacob-carlborg jacob-carlborg Feb 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but it's documented in the README.md

Yes, but we can remove that as well. It's a bit inconsistent to have unit_tests and the -u flag.

Btw currently there's no easy way in to run just the integration tests (i.e. compilable, runnable, fail_compilation). Maybe it's worth adding tests as a shortcut?

Perhaps.

./run.d all # runs all tests
./run.d clean # remove all test results
./run.d -u -- unit/deinitialization.d -f Module # runs the unit tests in the file "unit/deinitialization.d" with a UDA containing "Module"
Expand Down Expand Up @@ -123,7 +124,7 @@ Options:
ensureToolsExists(EnumMembers!TestTools);
foreach (target; taskPool.parallel(targets, 1))
{
log("run: %-(%s %)", target.args);
log("%-(%s %)", target.args);
ret |= spawnProcess(target.args, env, Config.none, scriptDir).wait;
}
if (ret)
Expand Down Expand Up @@ -156,7 +157,7 @@ void ensureToolsExists(const TestTool[] tools ...)
const targetBin = resultsDir.buildPath(tool).exeName;
const sourceFile = toolsDir.buildPath(tool ~ ".d");
if (targetBin.timeLastModified.ifThrown(SysTime.init) >= sourceFile.timeLastModified)
writefln("%s is already up-to-date", tool);
log("%s is already up-to-date", tool);
else
{
const command = [
Expand All @@ -165,8 +166,9 @@ void ensureToolsExists(const TestTool[] tools ...)
sourceFile
] ~ tool.extraArgs;

writefln("Executing: %-(%s %)", command);
spawnProcess(command).wait;
stderr.writefln("[run.d] %-(%s %)", command);
if (spawnProcess(command).wait != 0)
exit(1);
}
}

Expand Down Expand Up @@ -273,6 +275,8 @@ auto predefinedTargets(string[] targets)
foreach (testDir; testDirs)
newTargets.put(findFiles(testDir).map!createTestTarget);
break;
case "unittest":
case "unittests":
case "unit_tests":
newTargets ~= createUnitTestTarget();
break;
Expand All @@ -291,7 +295,7 @@ auto filterTargets(Target[] targets, string[string] env)
{
if (!target.exists)
{
writefln("Warning: %s can't be found", target.normalizedTestName);
stderr.writefln("[run.d] Warning: %s can't be found", target.normalizedTestName);
error = true;
}
}
Expand All @@ -305,7 +309,7 @@ auto filterTargets(Target[] targets, string[string] env)
auto resultRunTime = resultsDir.buildPath(testName ~ ".out").timeLastModified.ifThrown(SysTime.init);
if (!force && resultRunTime > testPath(testName).timeLastModified &&
resultRunTime > env["DMD"].timeLastModified.ifThrown(SysTime.init))
writefln("%s is already up-to-date", testName);
writefln("[run.d] %s is already up-to-date", testName);
else
targetsThatNeedUpdating ~= t;
}
Expand Down Expand Up @@ -413,7 +417,10 @@ string[string] getEnvironment()
auto log(T...)(T args)
{
if (verbose)
writefln(args);
{
args[0] = "[run.d]: " ~ args[0];
stderr.writefln(args);
}
}

// Add the executable filename extension to the given `name` for the current OS.
Expand Down
22 changes: 13 additions & 9 deletions test/tools/unit_test_runner.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import std.exception : enforce;
import std.file : dirEntries, exists, SpanMode, mkdirRecurse, write;
import std.format : format;
import std.getopt : getopt;
import std.path : absolutePath, buildPath, dirSeparator, stripExtension,
setExtension;
import std.path : absolutePath, buildPath, dirSeparator, relativePath,
setExtension, stripExtension;
import std.process : environment, spawnProcess, spawnShell, wait;
import std.range : empty;
import std.stdio;
Expand All @@ -19,6 +19,7 @@ import tools.paths;

enum unitTestDir = testPath("unit");
enum strtoldObjPath = resultsDir.buildPath("strtold.obj");
enum dmdSrcDir = projectRootDir.buildPath("src");

string[] testFiles(Range)(Range givenFiles)
{
Expand All @@ -34,10 +35,9 @@ string[] testFiles(Range)(Range givenFiles)
auto moduleNames(const string[] testFiles)
{
return testFiles
.map!(e => e[unitTestDir.length + 1 .. $])
.map!stripExtension
.array
.map!(e => e.substitute(dirSeparator, "."));
.map!(e => e.relativePath(unitTestDir)
.stripExtension
.substitute(dirSeparator, "."));
}

void writeRunnerFile(Range)(Range moduleNames, string path, string filter)
Expand Down Expand Up @@ -236,7 +236,7 @@ void writeCmdfile(string path, string runnerPath, string outputPath,
"-unittest",
"-J" ~ buildOutputPath,
"-J" ~ projectRootDir.buildPath("res"),
"-I" ~ projectRootDir.buildPath("src"),
"-I" ~ dmdSrcDir,
"-I" ~ unitTestDir,
"-i",
"-g",
Expand Down Expand Up @@ -289,10 +289,10 @@ void buildStrtold()
"/EHsc",
"/TP",
"/c",
projectRootDir.buildPath("src", "dmd", "backend", "strtold.c"),
dmdSrcDir.buildPath("dmd", "backend", "strtold.c"),
"/Fo" ~ strtoldObjPath,
"/I",
projectRootDir.buildPath("src", "dmd", "root")
dmdSrcDir.buildPath("dmd", "root")
].join(" ");

enforce(spawnShell(cmd).wait() == 0, "Failed to execute command: " ~ cmd);
Expand Down Expand Up @@ -321,6 +321,9 @@ int main(string[] args)
if (missingTestFiles(givenFiles))
return 1;

const nrOfFiles = givenFiles.length ? givenFiles.length.to!string : "all";
stderr.writefln("[unit] Starting to build %s test files", nrOfFiles);
Copy link
Contributor

@jacob-carlborg jacob-carlborg Feb 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why stderr?


enum runnerPath = resultsDir.buildPath("runner.d");
const testFiles = givenFiles.testFiles;

Expand All @@ -336,5 +339,6 @@ int main(string[] args)
buildStrtold();
execute(dmdPath, "@" ~ cmdfilePath);

stderr.writefln("[unit] Starting to run %s test files", nrOfFiles);
return spawnProcess(outputPath).wait();
Copy link
Contributor Author

@wilzbach wilzbach Feb 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably also prepend the output with a prefix here as it currently looks like this:

[unit] Starting to build all test files
 ... runnable/test9271.d            -fPIC ()
 ... runnable/printargs.d           -fPIC ()
...
 ... runnable/testfile.d            -fPIC ()
 ... runnable/b18034.d              -O -fPIC (-inline -release -g)
 ... runnable/test_dip1006.d        -check=in=off -check=out=off -check=invariant=off -fPIC ()
[unit] Starting to run all test files
1 tests, 0 failures
 ... runnable/mod1.d                -fPIC ()
 ... runnable/test15862.d           -O -release -fPIC ()

Copy link
Contributor

@jacob-carlborg jacob-carlborg Feb 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn’t that what your change is doing?

Copy link
Contributor Author

@wilzbach wilzbach Feb 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. The log is already from this change. We probably need to create our own Pipe and do sth. like:

while (!pid.tryWait.terminated) {
  // if pipe has new data, read it from the buffer, split by line and append `[unit]`
  sleep(1.msecs);
}

Copy link
Contributor

@jacob-carlborg jacob-carlborg Feb 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not following.

Copy link
Contributor Author

@wilzbach wilzbach Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spawnProcess directly forwards all stdout, stderr etc. We can't add [unit] prefix to the lines. This change is only added it to the log commands, not things like:

1 tests, 0 failures

Though for the user to know where the message came from imho it probably would be better to at least do sth. like:

[unit] 1 tests, 0 failures

For this we need to either add the prefix in the actual runner or e.g. read line-wise as discussed above from the stdout pipe of the spawned process.

Copy link
Contributor

@jacob-carlborg jacob-carlborg Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, now I get it.

}