Skip to content

Simplify interface to rdmd_test and make it more robust #331

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

Closed
marler8997 wants to merge 1 commit into from
Closed

Simplify interface to rdmd_test and make it more robust #331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 3 additions & 5 deletions posix.mak
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,19 +110,17 @@ test_tests_extractor: $(ROOT)/tests_extractor
$< -i ./test/tests_extractor/ascii.d | diff - ./test/tests_extractor/ascii.d.ext
$< -i ./test/tests_extractor/iteration.d | diff - ./test/tests_extractor/iteration.d.ext

RDMD_TEST_COMPILERS = $(abspath $(DMD))
RDMD_TEST_COMPILERS = $(DMD)
RDMD_TEST_EXECUTABLE = $(ROOT)/rdmd
RDMD_TEST_DEFAULT_COMPILER = $(basename $(DMD))

VERBOSE_RDMD_TEST=0
ifeq ($(VERBOSE_RDMD_TEST), 1)
override VERBOSE_RDMD_TEST_FLAGS:=-v
endif

test_rdmd: $(ROOT)/rdmd_test $(RDMD_TEST_EXECUTABLE)
$< --rdmd=$(RDMD_TEST_EXECUTABLE) -m$(MODEL) \
--rdmd-default-compiler=$(RDMD_TEST_DEFAULT_COMPILER) \
--test-compilers=$(RDMD_TEST_COMPILERS) \
$< $(RDMD_TEST_EXECUTABLE) -m$(MODEL) \
--compilers=$(RDMD_TEST_COMPILERS) \
$(VERBOSE_RDMD_TEST_FLAGS)
$(DMD) $(DFLAGS) -unittest -main -run rdmd.d

Expand Down
101 changes: 56 additions & 45 deletions rdmd_test.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ module rdmd_test;
While `rdmd_test` can be run directly, it is recommended to run
it via the tools build scripts using the `make test_rdmd` target.

When running directly, use the --rdmd flag to specify the path
to the rdmd executable, to test, and --rdmd-default-compiler to
specify the name of the default compiler expected by rdmd.
When running directly, pass the rdmd binary as the first argument.
*/

import std.algorithm;
Expand All @@ -28,6 +26,8 @@ import std.path;
import std.process;
import std.range;
import std.string;
import std.ascii : isWhite;
import std.stdio : writefln;

version (Posix)
{
Expand All @@ -48,42 +48,38 @@ else

bool verbose = false;

void main(string[] args)
int main(string[] args)
{
string rdmd; // path to rdmd executable
string defaultCompiler; // name of default compiler expected by rdmd
string compilers = "dmd"; // e.g. "ldmd2,gdmd" (comma-separated list of compiler names)
Copy link
Contributor

@WebDrake WebDrake Mar 18, 2018

Choose a reason for hiding this comment

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

I would suggest not hardcoding a default here. In the absence of anything being specified, it should default to rdmd's default compiler.

bool concurrencyTest;
string model = "64"; // build architecture for dmd
string testCompilerList; // e.g. "ldmd2,gdmd" (comma-separated list of compiler names)

auto helpInfo = getopt(args,
"rdmd", "[REQUIRED] path to rdmd executable to test", &rdmd,
"rdmd-default-compiler", "[REQUIRED] default D compiler used by rdmd executable", &defaultCompiler,
"compilers", "comma-separated list of D compilers to test rdmd with (defaults to 'dmd')", &compilers,
Copy link
Contributor

@WebDrake WebDrake Mar 18, 2018

Choose a reason for hiding this comment

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

This is a superfluous change that honestly looks more like bikeshedding over what the test-compilers flag (and internal variable) are called. I'd suggest dropping it since it distracts from the more important behavioural changes in this patch.

"concurrency", "whether to perform the concurrency test cases", &concurrencyTest,
"m|model", "architecture to run the tests for [32 or 64]", &model,
"test-compilers", "comma-separated list of D compilers to test with rdmd", &testCompilerList,
"v|verbose", "verbose output", &verbose,
);

void reportHelp(string errorMsg = null, string file = __FILE__, size_t line = __LINE__)
{
defaultGetoptPrinter("rdmd_test: a test suite for rdmd\n\n" ~
"USAGE:\trdmd_test [OPTIONS]\n",
"USAGE:\trdmd_test [OPTIONS] <rdmd-binary>\n",
helpInfo.options);
enforce(errorMsg is null, errorMsg, file, line);
}

if (helpInfo.helpWanted)
if (args.length == 1 || helpInfo.helpWanted)
Copy link
Contributor

@WebDrake WebDrake Mar 18, 2018

Choose a reason for hiding this comment

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

The various arguments-validation checks introduced here and below look harder to understand and maintain than just preserving the --rdmd flag. I'm not sure that the simplicity of being able to type

rdmd_test [PATH_TO_RDMD]

... is worth it when it's simple enough to type

rdmd_test --rdmd [PATH_TO_RDMD]

... especially bearing in mind that the makefile design is set up to avoid anyone having to run rdmd_test directly.

{
reportHelp();
return;
return 1;
Copy link
Contributor

@WebDrake WebDrake Mar 18, 2018

Choose a reason for hiding this comment

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

It would be simpler to pass an error message to reportHelp like "rdmd binary not specified!". That would avoid having to care about return values (and changing the main function definition).

}

if (rdmd.length == 0)
reportHelp("ERROR: missing required --rdmd flag");

if (defaultCompiler.length == 0)
reportHelp("ERROR: missing required --rdmd-default-compiler flag");
if (args.length > 2)
{
writefln("Error: too many non-option arguments, expected 1 but got %s", args.length - 1);
return 1;
Copy link
Contributor

@WebDrake WebDrake Mar 18, 2018

Choose a reason for hiding this comment

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

It would again be easier to report this error message via the reportHelp function (arguably it's nice to print the usage output alongside this error message and again, this avoids having to care about return values).

}
string rdmd = args[1]; // path to rdmd executable

enforce(rdmd.exists,
format("rdmd executable path '%s' does not exist", rdmd));
Expand All @@ -95,23 +91,22 @@ void main(string[] args)
scope (exit) std.file.remove(rdmdApp);
copy(rdmd, rdmdApp, Yes.preserveAttributes);

// if no explicit list of test compilers is set,
// use the default compiler expected by rdmd
if (testCompilerList is null)
testCompilerList = defaultCompiler;
runCompilerAgnosticTests(rdmdApp, model);

// run the test suite for each specified test compiler
foreach (testCompiler; testCompilerList.split(','))
foreach (compiler; compilers.split(','))
{
runTests(rdmdApp, testCompiler, model);
// if compiler is a relative filename it must be converted
// to absolute because this test changes directories
if (compiler.canFind!isDirSeparator || compiler.exists)
compiler = buildNormalizedPath(compiler.absolutePath);

runTests(rdmdApp, compiler, model);
if (concurrencyTest)
runConcurrencyTest(rdmdApp, testCompiler, model);
runConcurrencyTest(rdmdApp, compiler, model);
}

// run the fallback compiler test (this involves
// searching for the default compiler, so cannot
// be run with other test compilers)
runFallbackTest(rdmdApp, defaultCompiler, model);
return 0;
}

string compilerSwitch(string compiler) { return "--compiler=" ~ compiler; }
Expand All @@ -120,7 +115,6 @@ string modelSwitch(string model) { return "-m" ~ model; }

auto execute(T...)(T args)
{
import std.stdio : writefln;
if (verbose)
writefln("[execute] %s", args[0]);
return std.process.execute(args);
Expand All @@ -131,11 +125,8 @@ auto rdmdArguments(string rdmdApp, string compiler, string model)
return [rdmdApp, compilerSwitch(compiler), modelSwitch(model)];
}

void runTests(string rdmdApp, string compiler, string model)
void runCompilerAgnosticTests(string rdmdApp, string model)
{
// path to rdmd + common arguments (compiler, model)
auto rdmdArgs = rdmdArguments(rdmdApp, compiler, model);

/* Test help string output when no arguments passed. */
auto res = execute([rdmdApp]);
assert(res.status == 1, res.output);
Expand All @@ -146,11 +137,22 @@ void runTests(string rdmdApp, string compiler, string model)
assert(res.status == 0, res.output);
assert(res.output.canFind("Usage: rdmd [RDMD AND DMD OPTIONS]... program [PROGRAM OPTIONS]..."));

// run the fallback compiler test (this involves
// searching for the default compiler, so cannot
// be run with other test compilers)
runFallbackTest(rdmdApp, model, res.output);
}

void runTests(string rdmdApp, string compiler, string model)
{
// path to rdmd + common arguments (compiler, model)
auto rdmdArgs = rdmdArguments(rdmdApp, compiler, model);

/* Test --force. */
string forceSrc = tempDir().buildPath("force_src_.d");
std.file.write(forceSrc, `void main() { pragma(msg, "compile_force_src"); }`);

res = execute(rdmdArgs ~ [forceSrc]);
auto res = execute(rdmdArgs ~ [forceSrc]);
assert(res.status == 0, res.output);
assert(res.output.canFind("compile_force_src"));

Expand Down Expand Up @@ -589,17 +591,26 @@ void runConcurrencyTest(string rdmdApp, string compiler, string model)
}
}

void runFallbackTest(string rdmdApp, string buildCompiler, string model)
/* https://issues.dlang.org/show_bug.cgi?id=11997
if an explicit --compiler flag is not provided, rdmd should
search its own binary path first when looking for the default
compiler (determined by the compiler used to build it) */
void runFallbackTest(string rdmdApp, string model, string helpText)
{
/* https://issues.dlang.org/show_bug.cgi?id=11997
if an explicit --compiler flag is not provided, rdmd should
search its own binary path first when looking for the default
compiler (determined by the compiler used to build it) */
string localDMD = buildPath(tempDir(), baseName(buildCompiler));
std.file.write(localDMD, "empty shell");
scope(exit) std.file.remove(localDMD);
enum compilerHelpLine = " --compiler=comp use the specified compiler (e.g. gdmd) instead of ";
auto compilerHelpIndex = helpText.indexOf(compilerHelpLine);
assert(compilerHelpIndex >= 0);
auto defaultCompiler = helpText[compilerHelpIndex + compilerHelpLine.length .. $];
defaultCompiler = defaultCompiler[0 .. defaultCompiler.countUntil!isWhite];
if (verbose)
writefln("defaultCompiler is '%s'", defaultCompiler);
assert(defaultCompiler == "dmd" || defaultCompiler == "ldmd2" || defaultCompiler == "gdmd");
Copy link
Contributor

@WebDrake WebDrake Mar 18, 2018

Choose a reason for hiding this comment

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

This should be enforce, not assert, since it's validating external input, and because we'd always want the check to be performed.

However, note that this is introducing a maintenance burden, because it now means that assumptions about the possible choices of default compiler name are hardcoded into rdmd_test.


string compilerFilename = buildPath(tempDir(), defaultCompiler);
std.file.write(compilerFilename, "empty shell");
scope(exit) std.file.remove(compilerFilename);

auto res = execute(rdmdApp ~ [modelSwitch(model), "--force", "--chatty", "--eval=writeln(`Compiler found.`);"]);
assert(res.status == 1, res.output);
assert(res.output.canFind(`spawn ["` ~ localDMD ~ `",`));
assert(res.output.canFind(`spawn ["` ~ compilerFilename ~ `",`));
}
6 changes: 2 additions & 4 deletions win32.mak
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,13 @@ scp: detab tolf $(MAKEFILES)

RDMD_TEST_COMPILERS = $(DMD)
RDMD_TEST_EXECUTABLE = $(ROOT)\rdmd.exe
RDMD_TEST_DEFAULT_COMPILER = $(DMD)

$(ROOT)\rdmd_test.exe : rdmd_test.d
$(DMD) $(DFLAGS) -of$@ rdmd_test.d

test_rdmd : $(ROOT)\rdmd_test.exe $(RDMD_TEST_EXECUTABLE)
$(ROOT)\rdmd_test.exe \
--rdmd=$(RDMD_TEST_EXECUTABLE) -m$(MODEL) -v \
--rdmd-default-compiler=$(RDMD_TEST_DEFAULT_COMPILER) \
--test-compilers=$(RDMD_TEST_COMPILERS)
$(RDMD_TEST_EXECUTABLE) -m$(MODEL) -v \
--compilers=$(RDMD_TEST_COMPILERS)

test : test_rdmd