Route all tests through d_do_test by marler8997 · Pull Request #8129 · dlang/dmd

marler8997 · 2018-04-04T13:58:48Z

Provides a common entry point for all tests to perform common setup/operations. Also simplifies the interface to run a test (resulting in a simplified Makefile).

dlang-bot · 2018-04-04T13:58:48Z

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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#8129"

wilzbach · 2018-04-04T14:06:39Z

test/tools/d_do_test.d

-    string test_name      = args[2];
-    string test_extension = args[3];
+    auto test_file = args[1];
+    auto result = matchFirst(test_file, `^([^/^\\]+)[/\\]([^\.]+)\.(.+)$`);


Does this need to be RegExp? Doesn't baseName + stripExtension work too?

wilzbach · 2018-04-04T14:07:42Z

test/tools/d_do_test.d

+    {
+        auto process = spawnProcess(["./tools/sh_do_test.sh".replace("/", envData.sep),
+            input_dir, test_name]);
+        return process.wait();


Why not std.process.execute if you wait anyhow?

not capturing stdout/stderr

wilzbach

Nice & one step closer to making it easier for people on Windows to run the testsuite :)

wilzbach · 2018-04-04T15:48:34Z

test/tools/d_do_test.d

+    auto test_file = args[1];
+    string input_dir = test_file.dirName;
+    string test_base_name = test_file.baseName;
+    string test_name = test_base_name.stripExtension();


This is a bit inconsistent here. All three are functions ;-)
(by "your majority vote" maybe adapt stripExtension?)

wilzbach · 2018-04-04T15:49:51Z

test/tools/d_do_test.d

+    if (test_base_name.extension() == ".sh")
+    {
+        auto process = spawnShell(format("%s %s %s",
+            "./tools/sh_do_test.sh".replace("/", envData.sep), input_dir, test_name));


"tools".buildPath("sh_do_test.sh") ?

wilzbach · 2018-04-05T06:01:16Z

C:\projects\dmd\test\tools\sh_do_test.sh: line 17: /c/projects/dmd/test/exported_vars.sh: No such file or directory
C:\projects\dmd\test\tools\sh_do_test.sh: line 17: /c/projects/dmd/test/exported_vars.sh: No such file or directory
make[1]: *** [Makefile:129: test_results/runnable/test35.sh.out] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: *** [Makefile:129: test_results/runnable/test44.sh.out] Error 1
 ... runnable\ifti.d                  (-O -inline -g)
make: *** [Makefile:152: start_runnable_tests] Error 2

ibuclaw · 2018-04-05T06:36:49Z

Can't merge if there's failing tests, so that will need to be addressed first. Other than that, so far seems reasonable.

marler8997 · 2018-04-06T01:22:59Z

Closing and reopening to see if the appveryor failure is a red herring...

wilzbach · 2018-04-06T02:35:38Z

FYI I already restarted it twice before posting the error, so I think the error is a permanent one. It probably has something to do with AppVeyor's setup of Cygwin.

wilzbach · 2018-04-06T02:37:28Z

test/tools/sh_do_test.sh

    exit 1
 fi

-DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"


Why did you remove this?
This allows the script to be called from anywhere (in the future at least)

I think this might be why AppVeyor is failing...I"ve removed it to see if it fixes the issue... I know removing it is more "brittle". If this works, maybe I should redefined it to something like ${TEST_DIR}/../tools? I don't like it as much. The fact that Bash script files don't have an easy way to get their own directory is such a pain. This is one of the only things that BATCH has over Bash :) (%~dp0....done)

Adding bash to the beginning of the spawnShell command seems to have fixed the AppVeyor issue.

marler8997 · 2018-04-06T02:38:01Z

Yeah, I've been seeing a bunch of different errors on AppVeyor recently in other PRs but this one is consistent. Looks like it might be the definition of DIR in sh_do_test.sh might not be working for the AppVeyor...on to trial/error

Provides a common entry point for all tests to perform common setup/operations. Also simplifies the interface to run a test.

marler8997 force-pushed the commonTestEntry branch from 4ac2741 to 7953d4d Compare April 4, 2018 13:59

wilzbach reviewed Apr 4, 2018

View reviewed changes

marler8997 force-pushed the commonTestEntry branch 3 times, most recently from e171b64 to 3cdbcab Compare April 4, 2018 15:26

wilzbach approved these changes Apr 4, 2018

View reviewed changes

wilzbach added the Infrastructure:Build System label Apr 4, 2018

marler8997 force-pushed the commonTestEntry branch 3 times, most recently from d17b1c3 to 49458ea Compare April 4, 2018 15:59

wilzbach mentioned this pull request Apr 4, 2018

Port the individual test script runner to D #8136

Merged

JinShil approved these changes Apr 5, 2018

View reviewed changes

wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 5, 2018

ibuclaw removed the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 5, 2018

marler8997 closed this Apr 6, 2018

marler8997 reopened this Apr 6, 2018

marler8997 force-pushed the commonTestEntry branch 2 times, most recently from 60568b9 to 299a8e1 Compare April 6, 2018 02:28

wilzbach reviewed Apr 6, 2018

View reviewed changes

marler8997 force-pushed the commonTestEntry branch from 299a8e1 to 493af98 Compare April 6, 2018 02:48

Route all tests through d_do_test

c55fc62

Provides a common entry point for all tests to perform common setup/operations. Also simplifies the interface to run a test.

marler8997 force-pushed the commonTestEntry branch from 493af98 to c55fc62 Compare April 6, 2018 03:00

wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 6, 2018

JinShil added the Merge:auto-merge label Apr 7, 2018

dlang-bot merged commit a93a7ff into dlang:master Apr 7, 2018

Uh oh!

Comments

Conversation

marler8997 commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Apr 4, 2018

Bugzilla references

Testing this PR locally

Uh oh!

wilzbach Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

marler8997 Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Apr 5, 2018

Uh oh!

ibuclaw commented Apr 5, 2018

Uh oh!

marler8997 commented Apr 6, 2018

Uh oh!

wilzbach commented Apr 6, 2018

Uh oh!

wilzbach Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

marler8997 Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 Apr 6, 2018

Choose a reason for hiding this comment

Uh oh!

marler8997 commented Apr 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marler8997 commented Apr 4, 2018 •

edited

Loading

marler8997 Apr 6, 2018 •

edited

Loading