Use D to run the testsuite #8162

wilzbach · 2018-04-11T11:34:55Z

So there have been many complaints about how hard running the test suite on Windows is.
Here's an excerpt:

This PR allows to run the testsuite without GNUMake by making the run_individual_tests.d wrapper no longer dependent on the Makefile.
It's not perfect yet and I still have to test a few things, i.e.

can allow environment variables that can be overwritten in the Makefile, overwritten here too (e.g. results_dir)
use $HOST_DMD instead of dmd for building the tools
ensure that the correct $DMD and $MODEL is passed in everywhere
normalize the file names, s.t. this script can be run from anywhere
make sure it actually runs on Windows

but apart from that I think this already shows the general direction and it can already execute all tests, e.g.

./run_individual_tests.d fail_compilation
./run_individual_tests.d clean
./run_individual_tests.d all
./run_individual_tests.d fail_compilation -j10
./run_individual_tests.d fail_compilation/fail11562.d
./run_individual_tests.d fail_compilation AUTO_UPDATE=1

(I opened this already, s.t. no duplicate work on this happens)

dlang-bot · 2018-04-11T11:34:58Z

Thanks for your pull request, @wilzbach!

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 + dmd#8162"

marler8997 · 2018-04-11T16:23:35Z

test/run.d

+        env[key] = default_;
+}
+
+// Sets the environment variables required by d_do_test


// Sets the environment variables required by d_do_test and sh_do_test.sh

I anticipate that d_do_test might go away at some point (if Make goes away), but setting environment variables will likely still be required for the Bash tests.

marler8997 · 2018-04-11T16:51:07Z

test/run.d

+        }
+    }
+}
+


immutable testDirs = ["runnable", "compilable", "fail_compilation"];

Can be used in a few places.

marler8997 · 2018-04-11T16:51:42Z

test/run.d

+    }
+
+    // ensure output directories exist
+    foreach (dir; ["compilable", "fail_compilation", "runnable"])


foreach (dir; testDirs)

marler8997 · 2018-04-11T16:52:57Z

test/run.d

+    {
+        switch (t)
+        {
+            case "clean":


I had a problem with this, my tab-autocomplete added a trailing slash to the test directory (i.e. ./run.d compilable/). We could handle this by removing the trailing slash, maybe this?

if (t.endsWith(dirSeparator)) t = t[0 .. $ - 1];

marler8997 · 2018-04-11T16:53:36Z

test/run.d

+                break;
+
+            case "all":
+                newTargets.put(findFiles("runnable"));


foreach (testDir; testDirs) newTargets.put(findFinds(testDir));

marler8997 · 2018-04-11T16:54:34Z

test/run.d

+                break;
+
+            default:
+                newTargets ~= t;


This results in odd error messages in some cases. Maybe the following check would work well?

if (!testDirs.canFind(t.dirName)) { writefln("Error: invalid target '%s'", t); exit(1); }

We already check for file existence in filterTargets:

./run.d compilable/foo Warning: compilable/foo can't be found

What's the odd error message you get?

marler8997 · 2018-04-11T17:06:27Z

test/run.d

+        return "solaris";
+    else version(SunOS)
+        return "solaris";
+    else


No windows!??!! :)

I went ahead and tested that this runs on windows and it looks like it does. Just had to add the version(Windows) case here.

(windows is on the top of this list now)

marler8997 · 2018-04-11T17:09:45Z

Could we add a "v|verbose" flag to see things like the d_do_test commands that are being run?

marler8997 · 2018-04-12T10:10:08Z

Another thought...now that we have full control of the test suite, maybe it would be helpful to print a summary of the results at the end of the test. Something like:

----------------------------------------------
Test Result Summary
----------------------------------------------
failed runnable/test123.d
failed compilable/a.d
failed compilable/adfjalsdf.d
----------------------------------------------
100 Passed 20 Failed

wilzbach · 2018-04-29T19:51:52Z

Could we add a "v|verbose" flag to see things like the d_do_test commands that are being run?

Done. Though I only added it in a very naive way for now. We can always expand on this later.

Another thought...now that we have full control of the test suite, maybe it would be helpful to print a summary of the results at the end of the test. Something like:

Good idea, but an overkill for this PR. Let's focus on getting this to a mergeable state and let's expand on it afterwards?
It's already a rather large PR.

wilzbach

Okay I finally found time to work more on this and it should be ready now to replace the run_individual_tests.d script.
This improvement is a huge superset of the functionality of this test runner that we added a few weeks ago and hasn't been integrated in any CI process (except for a single test at CircleCi).
The long run goal is to replace the Makefile with run.d, but for now run.d should already allow running the tests on Windows and remove the dependence on GNUmake.

tl;dr: nothing depends on it + we can slowly improve run.d if there are still some slight differences between it and the Makefile (these probably won't be caught in the review anyhow).

wilzbach · 2018-04-29T20:53:42Z

test/run.d

+
+    // default target
+    if (!args.length)
+        args = ["all"];


Not sure on the default behavior. I'm leaning towards printing the help page if now arguments are passed (current behavior of the run_individual_tests.d tool.

I think that running all the tests by default is a more intuitive approach

wilzbach · 2018-04-29T20:59:32Z

test/run.d

+    foreach (t; targets)
+    {
+        if (!force && resultsDir.buildPath(t ~ ".out").timeLastModified.ifThrown(SysTime.init) >
+                scriptDir.buildPath(t).timeLastModified)


We could also check the timestamp of DMD or the build tools here too.

wilzbach · 2018-04-29T21:01:14Z

test/run.d

+        return "solaris";
+    else version(SunOS)
+        return "solaris";
+    else


(windows is on the top of this list now)

RazvanN7 · 2018-05-02T12:47:03Z

Looks like you forgot to delete the check for the run individual test. Delete it and we are ready to go. (this will solve the circleci failure)

RazvanN7 · 2018-05-02T12:51:52Z

Do we still need the Makefile after this? Maybe we should consider deleting it.

wilzbach · 2018-05-02T15:01:34Z

Do we still need the Makefile after this? Maybe we should consider deleting it.

No we don't :)
But currently a lot of CIs still depend on it, so the idea is slowly to migrate away from it.
Also, this new run.d test runner probably still needs a bit of real life testing and feedback.

Looks like you forgot to delete the check for the run individual test. Delete it and we are ready to go. (this will solve the circleci failure)

The failure was due to change of the file name. Changing it made the CI happy again and I think it doesn't hurt to keep testing this in the CI. After all, eventually we want all CIs to use it.

wilzbach · 2018-05-02T15:03:47Z

@marler8997 @JinShil are you two also okay with finally moving forward with this?
We can always work out the subtleties in follow-ups.

JinShil · 2018-05-03T00:42:17Z

test/run.d

+    if (!args.length)
+        args = ["all"];
+
+    alias normalizeTestName = f => format!"%s/%s"(f.absolutePath.dirName.baseName, f.baseName);


std.path.pathSeparator instead of /?

I went with buildPath

JinShil · 2018-05-03T00:46:05Z

test/tools/d_do_test.d

    }
    else
    {
-        auto process = spawnProcess(["./tools/sh_do_test.sh", input_dir, test_name]);
+        auto process = spawnProcess([scriptDir ~ "/tools/sh_do_test.sh", input_dir, test_name]);


The / will probably work fine in the posix/Bash environment. I don't know if it will or won't cause problems on Windows if we ever get to a point where we can run this outside of that environment. I suggest using std.path.pathSeparator.

I went with buildPath

JinShil · 2018-05-03T00:48:46Z

test/run.d

+                exit(0);
+                break;
+
+            case "run_runnable_tests", "runnable":


The "run_" prefix seems redundant, but I'm just being picky.

run_runnable_tests is the name of the existing Makefile target and at least for the transition I wanted to keep the most commonly used Makefile targets

JinShil

LGTM, but I'm wondering if the use of / as a delimiter will cause problems on Windows if run outside of a Posix/Bash environment. I'm also not sure how DMD handles / and \ when used as a separator in its arguments on Windows

JinShil · 2018-05-03T01:00:39Z

test/run.d

@@ -0,0 +1,342 @@
+#!/usr/bin/env rdmd
+/**
+Allows running tests individually


Probably not an entirely accurate summary anymore. It may be prudent to elaborate.

dlang-bot added the WIP Work In Progress - not ready for review or pulling label Apr 11, 2018

wilzbach force-pushed the d-make branch 2 times, most recently from 90942c0 to 8851c86 Compare April 11, 2018 12:00

marler8997 reviewed Apr 11, 2018

View reviewed changes

wilzbach force-pushed the d-make branch from 8851c86 to 6b9cb96 Compare April 13, 2018 12:32

wilzbach force-pushed the d-make branch 2 times, most recently from 661ae1d to fdabda4 Compare April 29, 2018 20:51

wilzbach removed the WIP Work In Progress - not ready for review or pulling label Apr 29, 2018

wilzbach force-pushed the d-make branch from fdabda4 to 2a4ef66 Compare April 29, 2018 20:52

wilzbach changed the title ~~[WIP] Use D to run the testsuite~~ Use D to run the testsuite Apr 29, 2018

wilzbach force-pushed the d-make branch from 2a4ef66 to a0e7d7c Compare April 29, 2018 21:01

wilzbach commented Apr 29, 2018

View reviewed changes

RazvanN7 approved these changes May 2, 2018

View reviewed changes

wilzbach force-pushed the d-make branch from a0e7d7c to 11ae581 Compare May 2, 2018 13:48

marler8997 approved these changes May 2, 2018

View reviewed changes

JinShil reviewed May 3, 2018

View reviewed changes

JinShil approved these changes May 3, 2018

View reviewed changes

JinShil reviewed May 3, 2018

View reviewed changes

wilzbach force-pushed the d-make branch 2 times, most recently from dbe4caf to e8626e4 Compare May 3, 2018 09:15

wilzbach mentioned this pull request May 3, 2018

Allow to run_individual_tests from any directory #8117

Closed

Use D to run the testsuite

a2f1a2e

wilzbach force-pushed the d-make branch from e8626e4 to a2f1a2e Compare May 4, 2018 15:37

JinShil added the auto-merge label May 7, 2018

dlang-bot merged commit f94107e into dlang:master May 7, 2018

wilzbach deleted the d-make branch May 25, 2018 06:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use D to run the testsuite #8162

Use D to run the testsuite #8162

wilzbach commented Apr 11, 2018 •

edited

Loading

dlang-bot commented Apr 11, 2018

marler8997 Apr 11, 2018 •

edited

Loading

marler8997 Apr 11, 2018

marler8997 Apr 11, 2018

marler8997 Apr 11, 2018 •

edited

Loading

marler8997 Apr 11, 2018

marler8997 Apr 11, 2018 •

edited

Loading

wilzbach Apr 29, 2018

marler8997 Apr 11, 2018

wilzbach Apr 29, 2018

marler8997 commented Apr 11, 2018

marler8997 commented Apr 12, 2018 •

edited

Loading

wilzbach commented Apr 29, 2018

wilzbach left a comment

wilzbach Apr 29, 2018

RazvanN7 May 2, 2018

wilzbach Apr 29, 2018

wilzbach Apr 29, 2018

RazvanN7 commented May 2, 2018 •

edited

Loading

RazvanN7 commented May 2, 2018

wilzbach commented May 2, 2018

wilzbach commented May 2, 2018

JinShil May 3, 2018

wilzbach May 3, 2018

JinShil May 3, 2018

wilzbach May 3, 2018

JinShil May 3, 2018

wilzbach May 3, 2018

JinShil left a comment •

edited

Loading

JinShil May 3, 2018

Use D to run the testsuite #8162

Use D to run the testsuite #8162

Conversation

wilzbach commented Apr 11, 2018 • edited Loading

dlang-bot commented Apr 11, 2018

Bugzilla references

Testing this PR locally

marler8997 Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marler8997 Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marler8997 Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marler8997 commented Apr 11, 2018

marler8997 commented Apr 12, 2018 • edited Loading

wilzbach commented Apr 29, 2018

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RazvanN7 commented May 2, 2018 • edited Loading

RazvanN7 commented May 2, 2018

wilzbach commented May 2, 2018

wilzbach commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JinShil left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Apr 11, 2018 •

edited

Loading

marler8997 Apr 11, 2018 •

edited

Loading

marler8997 Apr 11, 2018 •

edited

Loading

marler8997 Apr 11, 2018 •

edited

Loading

marler8997 commented Apr 12, 2018 •

edited

Loading

RazvanN7 commented May 2, 2018 •

edited

Loading

JinShil left a comment •

edited

Loading