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.
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
Small improvements to unit_tester_runner + run.d #9348
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request, @wilzbach! 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. Testing this PR locallyIf 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" |
@@ -336,5 +336,6 @@ int main(string[] args) | |||
buildStrtold(); | |||
execute(dmdPath, "@" ~ cmdfilePath); | |||
|
|||
stderr.writefln("[unit] Starting to run %s test files", nrOfFiles); | |||
return spawnProcess(outputPath).wait(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn’t that what your change is doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not following.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, now I get it.
BTW thinking more about the name |
@@ -321,6 +321,9 @@ int main(string[] args) | |||
if (missingTestFiles(givenFiles)) | |||
return 1; | |||
|
|||
const nrOfFiles = givenFiles.length ? givenFiles.length.to!string : "all"; | |||
stderr.writefln("[unit] Starting to build %s test files", nrOfFiles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why stderr?
What about “functional” or “integration”? Do we need to have it at all when we have |
@@ -67,6 +67,7 @@ Examples: | |||
./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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should not document it since the -u
flag exists as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I like "functional" as all the other tests (e.g. |
I would say that |
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. |
"integration" is the only other thing I can think of.
It all depends on what you defined as a unit 😃. |
This seem to have been forgotten. What's the status ? |
A few small follow-ups to #9333
See the individual commits.
CC @jacob-carlborg