Rework rdmd_test to support running test suite with multiple different compilers#293
Conversation
The original versions of these functions assume a single, global choice for compiler and model. By requiring the compiler or model choice to be passed as input we can more readily rework the rest of the test suite to test rdmd with different compiler or model choices.
|
Thanks for your pull request, @WebDrake! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
CyberShadow
left a comment
CyberShadow
left a comment
There was a problem hiding this comment.
Looks good so far. Thanks for the granular commits and extensive commit messages!
rdmd_test.d
Outdated
| // if no explicit list of test compilers is set, | ||
| // use the compiler used to build rdmd | ||
| if (testCompilerList is null) | ||
| testCompilerList = compiler; |
There was a problem hiding this comment.
Why not just initialize testCompilerList to compiler? That would make this check unnecessary.
There was a problem hiding this comment.
Oops, I see now that compiler is also retrieved from the command line.
|
I've made one small edit to the PR description noting that we could also extend this approach further to allow |
I think I can answer that. This is a test for rdmd checking its directory for a compiler to run by default; see the bug report linked above that code for a more detailed description, and the related discussion. The I suggest wrapping that test in a |
|
The aim of that test is to make sure rdmd uses a D compiler in the same directory it's in, if a |
Looking at this in more detail: /* https://issues.dlang.org/show_bug.cgi?id=11997- rdmd should search its
binary path for the compiler */
string localDMD = buildPath(tempDir(), baseName(compiler));
std.file.write(localDMD, "empty shell");
scope(exit) std.file.remove(localDMD);
res = execute(rdmdApp ~ [modelSwitch(model), "--force", "--chatty", "--eval=writeln(`Compiler found.`);"]);
assert(res.status == 1, res.output);
assert(res.output.canFind(`spawn ["` ~ localDMD ~ `",`));... it appears to create a "dummy" compiler which will be found, but which obviously will not "work". The idea is presumably to be able to demonstrate that The problem here is that absent a
TBH I think the correct solution here is to break that test out from To be really honest, this seems a bit of a flaky test -- how do you tell the difference between (i) |
That's what the I think the test doesn't even need to run the dummy compiler, and should work with |
If no explicit `--compiler` flag is passed, `rdmd` should first search for its default compiler (determined by the compiler used to build it) in the same directory that it itself lives in. The test of this behaviour is unique inasmuch as, unlike all other tests implemented by `rdmd_test`, it alone explicitly involves a case where `rdmd` is searching for a single compiler name. It must therefore be separated out from all the other tests if we want to be able to rework them to run with multiple different compilers (which clearly requires use of the `--compiler` flag). The new `runFallbackTest` function has been implemented so as to avoid having any dependencies on global state, since this will simplify future changes required by multi-compiler tests.
The existing rdmd_test script assumes that the compiler invoked by rdmd when running tests is the same as the compiler used to build rdmd, and that it targets the same build model. This helper function should make it easier to construct tests that use different compiler and/or model settings.
This should allow the test suites to be run for multiple different rdmd, compiler and model choices, and reduce dependencies on global variables.
This makes it easier to ensure that the test suite can be run for many different choices of these parameters. The `verbose` parameter has been left as global because it seems less likely that this might need to be customizable for different runs of the test suite.
This new flag can be used to specify the list of different compilers for
`rdmd` to invoke when running the test suite. This can be provided as a
single compiler name, or as a comma-separated list, e.g.
--test-compilers=dmd,gdmd,ldmd2
The complete test suite will be run for each of the specified compilers.
If the flag is not supplied, then the test suite will be run for the
same compiler as was used to build `rdmd` (as specified by `rdmd_test`'s
`--compiler` flag).
The test for the fallback to default compiler is still invoked only with
respect to the build compiler, since by its very nature this involves a
search for only the one compiler name (determed when `rdmd` is built).
This is a little bit bikesheddy but helps to emphasize the distinction between the compiler used to build rdmd, and the compiler(s) invoked by rdmd when running the test suite. Note that the `--compiler` flag of `rdmd_test` has been left unchanged so as not to risk breaking changes for users.
WebDrake
force-pushed
the
test-compiler-flag
branch
from
8671a67 to
da1fdab
Compare
|
I've updated the PR with one new patch (early in the series, even though it shows up last in GitHub) that breaks out the fallback test into a separate function. This seems the cleanest way of clearly separating it from everything that can be run with a specified The overall diff from the old PR is: diff --git a/rdmd_test.d b/rdmd_test.d
index c728454..f0edf9e 100755
--- a/rdmd_test.d
+++ b/rdmd_test.d
@@ -88,6 +88,11 @@ void main(string[] args)
if (concurrencyTest)
runConcurrencyTest(rdmdApp, testCompiler, model);
}
+
+ // run the fallback compiler test (this involves
+ // searching for the build compiler, so cannot
+ // be run with other test compilers)
+ runFallbackTest(rdmdApp, buildCompiler, model);
}
string compilerSwitch(string compiler) { return "--compiler=" ~ compiler; }
@@ -342,16 +347,6 @@ void runTests(string rdmdApp, string compiler, string model)
assert(res.status == -SIGSEGV, format("%s", res));
}
- /* https://issues.dlang.org/show_bug.cgi?id=11997- rdmd should search its
- binary path for the compiler */
- string localDMD = buildPath(tempDir(), baseName(compiler));
- std.file.write(localDMD, "empty shell");
- scope(exit) std.file.remove(localDMD);
-
- res = execute(rdmdApp ~ [modelSwitch(model), "--force", "--chatty", "--eval=writeln(`Compiler found.`);"]);
- assert(res.status == 1, res.output);
- assert(res.output.canFind(`spawn ["` ~ localDMD ~ `",`));
-
/* -of doesn't append .exe on Windows: https://d.puremagic.com/issues/show_bug.cgi?id=12149 */
version (Windows)
@@ -571,3 +566,18 @@ 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) */
+ string localDMD = buildPath(tempDir(), baseName(buildCompiler));
+ std.file.write(localDMD, "empty shell");
+ scope(exit) std.file.remove(localDMD);
+
+ auto res = execute(rdmdApp ~ [modelSwitch(model), "--force", "--chatty", "--eval=writeln(`Compiler found.`);"]);
+ assert(res.status == 1, res.output);
+ assert(res.output.canFind(`spawn ["` ~ localDMD ~ `",`));
+}This should fix the concerns about the test without any breaking change, and also ensure that no point in the commit series introduces the opportunity to run tests in an invalid way. |
Ah, gotcha. I didn't look into what & thanks both @CyberShadow and @joakim-noah for clarifying the goals and requirements of the fallback test! |
|
Just for reference: I expect to be AFK for a couple of days now, so expect to next hear from me Wednesday at the earliest. If the PR is good enough to take in as it stands, happy days ;-) |
|
LGTM! @wilzbach, any comments? |
|
Just to make sure it doesn't get overlooked: would you like a changelog entry? |
... ping? :-) I'm reunited with my computer, so can make any last tweaks anyone would like now. |
|
As this doesn't directly affect D users, I don't think a changelog entry is necessary. |
|
I guess there is nothing left to say. |
|
Thanks! |
4 participants
This patchset reworks the test-suite functions inside
rdmd_testto allow them to be run with an arbitrary choice ofrdmdexecutable, D compiler invoked byrdmd, and target model (32- or 64 bits). These new features are then used to implement a new--test-compilersflag, which allows the user to specify a comma-separated list of different compiler names to run the test suite with. If no test compilers are specified, the test suite will be run using the same compiler as used to buildrdmd(the "build compiler").This should result in identical behaviour of
rdmd_testin cases where--test-compilersare not specified, but will allow CI to be updated in future to ensure thatrdmdis tested with all the compilers thatrdmdshould support. This should help to avoid repeats of situations like that observed in #271, where breaking changes inadvertently introduced would have been spotted immediately ifrdmdtests had been run with multiple different compilers.No changes have been made to actual CI for now, but once this code lands it should be fairly straightforward to update makefiles and
travis.shto ensure that multiple different test compilers are used.No updates have been made to
changelog.ddsince I'm not sure what reporting is expected forrdmd_test, but that can be updated easily if desired.Note that the approach here could be extended still further to implement
--test-modelsin future, but since testing compiler support is the focus of interest at the current moment, this is left for a later decision.Important: the test on L351 of the final code currently breaks if
ldmd2is used as a test compiler. It's not 100% obvious what the intention is behind this test: it very explicitly excludes the use of the--compilerflag tordmd, and no attempt has been made to change this, since without a clearer sense of the intentions behind this test, it seems better to preserve the pre-existing test than to risk breaking it for the sake of a new use-case.The original code:
... was apparently written by @joakim-noah: can you possibly comment on this? (This PR tweaks the
modelSwitchcall to reflect the fact thatmodelis now an input parameter to therunTestsfunction, rather than a global.)In the bigger picture: the design of
rdmd_testseems a little bit odd inasmuch as it builds its ownrdmdrather than running tests using an already-builtrdmdinstance. It's not obvious to why this is so but it looks a little problematic (don't we want to testrdmdas built by the standard build scripts?). No attempt has been made to address this for now, but it may be worth considering in future.