Small improvements to unit_tester_runner + run.d by wilzbach · Pull Request #9348 · dlang/dmd

wilzbach · 2019-02-10T15:54:29Z

A few small follow-ups to #9333
See the individual commits.

CC @jacob-carlborg

dlang-bot · 2019-02-10T15:54:30Z

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

wilzbach · 2019-02-10T15:57:28Z

test/tools/unit_test_runner.d

    execute(dmdPath, "@" ~ cmdfilePath);

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


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 ()

Isn’t that what your change is doing?

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); }

Sorry, I'm not following.

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.

Ah, ok, now I get it.

wilzbach · 2019-02-10T15:59:16Z

BTW thinking more about the name unit_tests, it might be that someone gets confused that it doesn't run the unittests within DMD?

jacob-carlborg · 2019-02-10T16:39:33Z

test/tools/unit_test_runner.d

        return 1;

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


Why stderr?

jacob-carlborg · 2019-02-10T16:41:56Z

BTW thinking more about the name unit_tests, it might be that someone gets confused that it doesn't run the unittests within DMD?

What about “functional” or “integration”? Do we need to have it at all when we have -u? It was mostly to be compatible with make.

test/tools/unit_test_runner.d

jacob-carlborg · 2019-02-10T20:15:41Z

test/run.d

    ./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


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

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?

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.

wilzbach · 2019-02-12T11:49:39Z

What about “functional” or “integration”?

I like "functional" as all the other tests (e.g. runnable) are integration tests too.
Other opinions?

jacob-carlborg · 2019-02-12T11:56:02Z

I like "functional" as all the other tests (e.g. runnable) are integration tests too.

I would say that runnable, and the others, are end-to-end tests. But apparently this site [1] defines "functional" and "end-to-end" to be the same thing.

[1] https://codeutopia.net/blog/2015/04/11/what-are-unit-testing-integration-testing-and-functional-testing/

kinke · 2019-02-26T17:06:24Z

I can live with 'functional' (can't think of something better); anything is better than the current name, which I found really confusing. ;)

As a side note, this runner will need some adapation to be useful for LDC, due to the amount of hardcoded paths.

jacob-carlborg · 2019-02-26T19:59:10Z

I can live with 'functional' (can't think of something better);

"integration" is the only other thing I can think of.

anything is better than the current name, which I found really confusing. ;)

It all depends on what you defined as a unit 😃.

Geod24 · 2020-03-31T19:39:20Z

This seem to have been forgotten. What's the status ?

wilzbach added 3 commits February 10, 2019 16:21

Document unit_tests + add synonyms

90ce29e

Add logging to the unit_test_runner

c23f497

Use relativePath instead of a hard-coded replace

61effc9

wilzbach added the Review:Easy Review label Feb 10, 2019

wilzbach commented Feb 10, 2019

View reviewed changes

wilzbach force-pushed the unit_test branch from 57ca2b8 to e304e70 Compare February 10, 2019 16:01

Mark logging in run.d with [run.d]

306a5aa

wilzbach force-pushed the unit_test branch from 3104068 to 534b5c5 Compare February 10, 2019 16:18

jacob-carlborg reviewed Feb 10, 2019

View reviewed changes

jacob-carlborg suggested changes Feb 10, 2019

View reviewed changes

test/tools/unit_test_runner.d Outdated Show resolved Hide resolved

test/tools/unit_test_runner.d Outdated Show resolved Hide resolved

jacob-carlborg reviewed Feb 10, 2019

View reviewed changes

Use dmdSrcDir for projectDir.buildPath('src', 'dmd')

a31a60c

wilzbach force-pushed the unit_test branch from 534b5c5 to a31a60c Compare February 12, 2019 10:31

dlang-bot added Review:Needs Rebase Review:stalled labels May 20, 2021

dlang-bot added the Review:Needs Work label Jan 16, 2023

dlang-bot added Merge:needs rebase stalled labels Dec 4, 2024

Uh oh!

Conversation

wilzbach commented Feb 10, 2019

Uh oh!

dlang-bot commented Feb 10, 2019

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Feb 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented Feb 10, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Feb 12, 2019

Uh oh!

jacob-carlborg commented Feb 12, 2019

Uh oh!

kinke commented Feb 26, 2019

Uh oh!

jacob-carlborg commented Feb 26, 2019

Uh oh!

Geod24 commented Mar 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants