Modify unittest handling by schveiguy · Pull Request #1685 · dlang/druntime

schveiguy · 2016-10-27T18:26:41Z

Currently, if unit tests run and all pass, the program is started. Typically users expect the main function to exit immediately, so they instrument their main program with

version(unittest) { void main() {} }
else int main(string[] args)
{
   ...
}

But this sucks. D shouldn't be running the main program after unit testing (by default). This can be an opt-in for the user, but should default to exiting.

This modification changes the unit test handler to return int instead of bool to signify different handling of the unit test result. If the handler function returns 0, then the main function is run. If the handler returns nonzero, the main function is not run. If the result is int.min, then the program exits with a failure exit code. If the result is negative, the program prints the negated value as the number of modules that failed, and exits with a failure exit code. If the result is positive, that number is printed as the number of modules that were tested, and exits with a success exit code.

the API for setting the handler routine should be backwards compatible, as I allow setting of the handler routine as a bool return or an int return. The one place where it may be incorrect is if you fetch the handler routine and you set a bool version. This code casts it to an int-returning routine. The effect may be very negligible, as I believe both would return using a register. I did the cast mainly to allow code that checks to see which exact routine is installed will work. I'm expecting there's almost no usage of the getter property there anyway.

Possible improvements -- we may want to only enable this behavior with a -DRT switch. Or we may want to allow the previous behavior with such a switch. We may want to separate the bool/int handlers into a different API so the getter makes more sense if complaints arise.

I'm adding this for testing and also to allow for discussion.

schveiguy · 2016-10-27T18:38:59Z

ping @wilzbach circleci seems very sensitive to line number changes. Can we fix this? I don't like the idea of having the build break because you added some lines of code!

jacob-carlborg · 2016-10-27T19:14:59Z

Not sure I fully follow when the different int values will be used, but can't you use an enum?

schveiguy · 2016-10-27T19:19:55Z

Not sure I fully follow when the different int values will be used

So let's say 5 modules are tested. In the default routine, if all 5 unit test routines pass, then the function returns 5. Then the runtime sees that some unit tests were run, prints "5 Module unit tests passed", and exits.

If 2 of the module unit tests fail, stack traces are printed, then the routine returns -2. The runtime then prints "2 Module unit tests failed", and exits.

I could change to enum, but then I'll just print "All unit tests passed". The extra info of how many modules passed may be trivia, but it may also be useful. I'm open to being convinced 😉

schveiguy · 2016-10-27T19:23:17Z

Fixed issue with trace expectation. This will probably fix circleci tests too.

jacob-carlborg · 2016-10-27T19:26:03Z

I see. Another thing, how does this work with custom unit test runners? For example, a unit test runner that knows how many actual unit tests were run instead of the number of tested modules. That unit test runner would most likely not want print the number of modules tested.

schveiguy · 2016-10-27T19:28:56Z

The custom runner can return 0 or int.min, and nothing will be printed. However, you may want the runtime not to run main, but still return 0 without printing anything. That case isn't handled. Perhaps int.max should do that.

schveiguy · 2016-10-27T19:31:15Z

Perhaps int.max should do that

Now does that. Code looks more even also.

schveiguy · 2016-10-27T19:32:26Z

BTW, part of the WIP, I need to add testing for these cases.

jacob-carlborg · 2016-10-27T19:34:15Z

The int.max addition should be documented as well.

schveiguy · 2016-10-27T19:56:44Z

@jacob-carlborg updated docs.

jacob-carlborg · 2016-10-27T20:00:05Z

src/core/runtime.d

     * value of this routine indicates to the runtime whether the tests ran
     * without error.
     *
+     * There are two options for handlers. The `bool` version is deprecated but


Shouldn't this say "schedule for deprecation" unless deprecated is added to the deprecated overload?

This is going to be permanently left in place, in my opinion. It doesn't cause much grief, the translation to the new version is backwards compatible, and so we don't have to do anything more.

We may want to simply undocument it at some point.

Ok, fair enough.

deadalnix · 2016-10-27T23:07:46Z

I'm not convinced this is the right approach. The thing will still fail to link when no main function is provided.

IMO, it is better to do this as proposed by basil. I plan to do a DMD PR.

schveiguy · 2016-10-28T14:05:03Z

The thing will still fail to link when no main function is provided.

dmd -main should work just fine.

wilzbach · 2016-12-11T19:56:42Z

ping @wilzbach circleci seems very sensitive to line number changes. Can we fix this? I don't like the idea of having the build break because you added some lines of code!

This is the intended behavior. There is no "forced percentage" on the code coverage results, it is a mere help for the reviewers that they maybe should have a closer look at the untested lines.
Of course it can be ignored.

Btw in case someone missed the info, there's a CodeCov browser extension for user convenience. For example in the diff view it will highlight all the missed lines in red ;-)

schveiguy · 2016-12-16T18:06:38Z

@wilzbach the issue actually wasn't circleci, it was the build itself (see my later comment). I didn't realize this right away, so false alarm!

andralex · 2016-12-25T16:10:58Z

src/core/runtime.d

+     *
+     * The default unit tester does not return `int.min` or `int.max`, but
+     * rather the number of tests passed or failed (or 0 if none are run).
+     *


OK, so for the new handler:

0 means all good run main

int.min failure with no extra information

int.max all good don't run main

0 these many unittests passed, none failed, don't run main

<0 the negation of these many unittests failed, print and exit(1)

That's subtle but nothing we can't handle. How about an enum as was suggested already:

How about this:

enum UnittestResult { failStop = int.min, successStop, successContinue = 0 }

Then everything outside these values would obey the <0 / >0 rules. For nice symmetry successStop could be int.max.

I will still make the return be int, but also define an enum that can be returned for convenience.

andralex · 2016-12-25T16:12:29Z

src/core/runtime.d

    static @property ModuleUnitTester moduleUnitTester()
    {
+        if(sm_moduleUnitTester == null)
+            return cast(ModuleUnitTester)sm_boolModuleUnitTester;


Instead of the cast use a small lambda?

So I'm a bit concerned about doing it that way. I don't know what code exists out there, but there is the possibility that code is checking to see if their particular unit tester is set. If I wrap in a lambda, that will always be false.

Now, code that checks against the function address of wrong type is going to fail to compile. But there will be no way to rectify the situation. At least with the way I have implemented, you can cast to the correct type to figure out if you've set the function.

Alternatively, I can just expose the "new" style unit tester via a new property (and make them mutually exclusive).

Also note, that there isn't any danger of improperly calling the function, since it takes no parameters. Just the return type is different, and won't crash the code that uses the wrong function type.

andralex · 2016-12-25T16:13:30Z

src/core/runtime.d

+                catch( Throwable e )
+                {
+                    _d_print_throwable(e);
+                    --failed;


cool :) prolly clearer if the name were negativeFailed

I will fix the code to increment instead (and return -failed at the end).

andralex · 2016-12-25T16:14:24Z

src/rt/dmain2.d

+            else if (utResult > 0)
+            {
+                if(utResult != int.max)
+                    .fprintf(.stderr, "%d Module unit tests passed\n", utResult);


use the actual "unittest" keyword:

.fprintf(.stderr, "%d unittests passed\n", utResult);

andralex · 2016-12-25T16:14:44Z

src/rt/dmain2.d

+            else
+            {
+                if (utResult != int.min)
+                    .fprintf(.stderr, "%d Module Unit tests failed\n", -utResult);


similar here, maybe FAILED in uppercase

schveiguy · 2016-12-25T16:15:15Z

I will finish this up with some testing soon!

andralex · 2016-12-25T16:15:38Z

So this would be complemented by a separate flag (e.g. '-unittest-only') in the compiler.

CyberShadow · 2016-12-26T11:39:33Z

So this would be complemented by a separate flag (e.g. '-unittest-only') in the compiler.

Would it be possible to make it so that DMD's -main and rdmd's --main, when combined with -unittest, both continue to do the right thing after the change?

andralex · 2016-12-26T12:43:55Z

@CyberShadow I was thinking one uses either -unittest or -unittest-only` but not both.

schveiguy · 2016-12-27T02:24:36Z

Made suggested edits.
Added test harness for custom unittest handler
Added support for --DRT-unittestmode option:

runmain: run main even on passing unittests (currently default, change in 2 versions).
unittestonly: if any unittests present, do not run main. (will be default on 2.074)

schveiguy · 2016-12-27T02:25:22Z

Note, automated coverage is not going to work here, because of the nature of how we have to test these things. The added tests should cover all the new code however.

jacob-carlborg · 2016-12-27T08:09:33Z

src/core/runtime.d

+    else switch (rt_configOption("unittestmode"))
+    {
+    case "":
+        // By default, run main. Switch to only doing unit tests in 2.074


~~Could we please call this run-main~~.

Assuming you are backing out of that request?

Yes. I accidentally commented on the wrong line. I left the comment because I hate when I get a notification and then I get find it on GitHub because it was removed.

jacob-carlborg · 2016-12-27T08:11:25Z

src/core/runtime.d

-    return Runtime.sm_moduleUnitTester();
+    import rt.config : rt_configOption;
+    if (failed != 0)
+        return -failed;


Could we please call this unit-test-mode or unittest-mode.

I'm game for anything in terms of naming, I agree it's long but I couldn't think of something better. My original does follow precedent of all lowercase no-space or hyphen.

CC @MartinNowak thoughts?

I don't think the name is too long, I just like to have a hyphen (or something else) to separate the words. I see three existing usages of rt_configOption, two with lowercase no-space or hyphen gcopt and oncycle and one with camel case no-space or hyphen callStructDtorsDuringGC.

Hm... if there is precedent, I'd prefer the camel case. I was going off this comment when I was doing the cycle detection parameters.

I cannot recall ever seeing a program using camel case for the command line flags, most use a hyphen. I don't feel strongly about it, I just thought we should do what most other programs are doing.

I agree with @jacob-carlborg here. Command line flags usually use a hyphen.

jacob-carlborg · 2016-12-27T08:14:44Z

src/rt/dmain2.d

+            }
+            else if (utResult > 0)
+            {
+                if(utResult != int.max)


Space after if

How about using the enum instead of int.max/min

Space after if

ok

How about using the enum

The enum is in the public interface, not imported here. I could re-create the enum, but I don't think it's worth doing that. I will put a comment to clarify that it is attached to the enum.

The enum is in the public interface, not imported here. I could re-create the enum, but I don't think it's worth doing that. I will put a comment to clarify that it is attached to the enum.

Works for me. I didn't noticed they were in separate modules.

schveiguy · 2016-12-27T14:45:24Z

Fixed formatting nit, and added comments to clarify enum usage. Also squashed into 2 commits.

jacob-carlborg · 2017-11-30T20:32:30Z

src/core/runtime.d

+    /**
+     * Should we print a summary of the results. Ignored if 0 tests executed.
+     */
+    bool summarize;


I'm not sure what the summary actually contains but many frameworks print the number of tests run even if 0 tests were run.

OK, so D's determination if we are in "unittest mode" is whether there exists any unit tests at all. So if there are no unit tests run, the runtime thinks that you are not testing.

But thinking about this some more, we can easily add an option for "don't ever run main, even without unit tests" as well, as we are already processing a runtime switch. I'll add it, and let you know.

BTW, the summary is done in the dmain2 file, I added it in this PR, look for the printf statements

jacob-carlborg · 2017-11-30T20:37:03Z

src/core/runtime.d

+ * the original behavior of the unit testing system.
+ *
+ * If no unittest custom handlers are registered, the following algorithm is
+ * executed (the behavior can be affected by the `--DRT-unittestmode` switch


I'm not sure what the naming conventions are when it comes to --DRT flags but I think it should be called --DRT-unit-test-mode since that's what most Posix commands use.

jacob-carlborg · 2017-11-30T20:38:19Z

src/core/runtime.d

+ *
+ * If the --DRT-unittestmode is set to "runmain", then even if unit tests are
+ * run (and all pass), main is still run. This is currently the default. If
+ * --DRT-unittestmode is set to "unittestonly", then any unit tests present


Same here, I think unittestonly should be unit-test-only.

well unittest is already considered a special word so prolly unittest-only etc

OK, will do.

dlang-bot · 2017-12-01T03:57:24Z

Thanks for your pull request, @schveiguy!

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.

schveiguy · 2017-12-01T03:58:54Z

I added a third option for the DRT parameter. I changed the parameter to just say "test" and not "unittest", since it was very verbose otherwise. Let me know what you think. I still need to make a changelog entry.

jacob-carlborg · 2017-12-01T09:57:17Z

src/core/runtime.d

+     *  The current  module unit tester handler or null if none has been
+     *  set.
+     */
+


Please remove empty newline between Ddoc comment and method declaration. It's not consistent with the already existing declarations.

jacob-carlborg · 2017-12-01T09:59:33Z

src/core/runtime.d

     * }
     * ---------
     */
+    static @property void extModuleUnitTester( ExtendedModuleUnitTester h )


I would prefer that the name, ext, was not shortened. Most likely you only going to call this method once, there's no reason to make it less clear to save a few letters.

I wished to call it moduleUnitTester, but since there is an accessor, I'm reluctant to do that (the previous version of this PR did that, but the function pointer was returning a value compatible with bool).

I can change this.

jacob-carlborg · 2017-12-01T10:01:19Z

src/core/runtime.d

+ * the original behavior of the unit testing system.
+ *
+ * If no unittest custom handlers are registered, the following algorithm is
+ * executed (the behavior can be affected by the `--DRT-testmode` switch


I would have hoped for --DRT-testmode to be renamed to --DRT-test-mode as well.

So far, the DRT options are gcopt, oncycle, and it looks like some other newer camelCase ones: callStructDtorsDuringGC, scanDataSeg

So I don't know which precedent to follow. ping @MartinNowak @andralex

I'll note none of the precedents have dashes.

I try to follow what I see in most command line applications. In these case I like to look at how Git is doing things. Git contains quite a lot of subcommands and flags. There's also this, which I think is from the Posix standard.

"The arguments that consist of characters and single letters or digits" [1].

[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html

I can't recall that I've event seen command line flags that use camel casing.

jacob-carlborg · 2017-12-01T10:01:56Z

src/core/runtime.d

+    }
+    else switch (rt_configOption("testmode"))
+    {
+    case "":


Shouldn't everything in this block be indented one level?

Hm... I generally don't indent switch case labels. I'm not sure what the style guide says. I see switch statements elsewhere that don't have case labels indented: https://github.com/dlang/druntime/blob/master/src/core/time.d#L352

Yeah, I was surprised now to see how many place in Phobos that don't indent cases in switch statements. I always indent the content of a block.

jacob-carlborg · 2017-12-01T10:02:40Z

src/rt/dmain2.d

+        {
+            auto utResult = runModuleUnitTests();
+            assert(utResult.passed <= utResult.executed);
+            if(utResult.passed == utResult.executed)


Space after if before the opening parenthesis .

jacob-carlborg · 2017-12-01T10:12:59Z

src/rt/dmain2.d

+            else
+            {
+                if (utResult.summarize)
+                    .fprintf(.stderr, "%d/%d unittests FAILED\n", cast(int)(utResult.executed - utResult.passed), cast(int)utResult.executed);


Most test frameworks will print the same summary regardless if something failed or not. That is, they usually print the total number of tests, the number of passed and the number of failed. I'm not sure how we would like it to behave.

If nothing failed, then the number of tests run and passed is printed (a few lines above this).

This behavior is totally up for grabs if someone wants to make the effort to repaint the bikeshed. My goal is simply to stop the runtime from always executing main after running unit tests.

jacob-carlborg · 2017-12-01T10:16:47Z

I added a third option for the DRT parameter.

👍

I changed the parameter to just say "test" and not "unittest", since it was very verbose otherwise.

I like that, then we don't have to argue if it should be unit-test or unittest 😃.

jacob-carlborg · 2017-12-01T10:17:22Z

Please squash all commits when done.

andralex · 2017-12-05T00:25:22Z

src/core/runtime.d

+     *    true if execution should continue after testing is complete, false if
+     *    not.
+     */
+    bool opCast(T : bool)() const


Isn't this too cute? A named method should be better.

It's not to be cute :)

It's to not break code. Existing code that may have their own runtime startup could easily be written like:

if(runModuleUnitTests()) { dmain(); }

andralex · 2017-12-05T00:28:16Z

src/core/runtime.d

+     *     if(result.executed != result.passed)
+     *         result.summarize = true; // print failure
+     *     else
+     *         result.runMain = true; // all UT passed


result.runMain = result.executed != result.passed;

Hm... I didn't set summarize or runMain if they were to be false (I did originally, similarly to what you had, but realized it defaults to false anyway). I can be more explicit.

Oops, didn't realize this was the example. The real code is more complex due to the DRT options. I've updated it anyway.

andralex · 2017-12-05T13:13:25Z

src/core/runtime.d

     *         }
     *     }
-     *     return failed == 0;
+     *     if(result.executed != result.passed)


space after if, mmmmkay? :)

You could use torture on me, and I still won't ever remember to do this 😆

tests are run (at all) and no failures occurred (with nice message).

dlang-bot · 2017-12-05T13:39:10Z

Thanks for your pull request, @schveiguy!

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.

dlang-bot · 2017-12-05T13:41:44Z

Thanks for your pull request, @schveiguy!

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.

schveiguy · 2017-12-05T13:43:40Z

OK, I think this is good. Please let me know if I missed any spacing issues!
I added a changelog entry, please review also.

Fix nits in PR review

dlang-bot · 2017-12-05T13:46:47Z

Thanks for your pull request, @schveiguy!

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.

schveiguy force-pushed the exitafterunittest branch from 8536fd5 to adf5b2c Compare October 27, 2016 19:35

jacob-carlborg reviewed Oct 27, 2016

View reviewed changes

wilzbach mentioned this pull request Dec 24, 2016

Don't run main after unittests #1724

Closed

andralex suggested changes Dec 25, 2016

View reviewed changes

schveiguy changed the title ~~[WIP] Modify unittest handling~~ Modify unittest handling Dec 27, 2016

jacob-carlborg reviewed Dec 27, 2016

View reviewed changes

schveiguy force-pushed the exitafterunittest branch from 9e919d9 to 5dcdcd0 Compare December 27, 2016 14:44

jacob-carlborg reviewed Nov 30, 2017

View reviewed changes

jacob-carlborg reviewed Dec 1, 2017

View reviewed changes

andralex approved these changes Dec 5, 2017

View reviewed changes

andralex reviewed Dec 5, 2017

View reviewed changes

Alter API (backwards compatible) for unit testing, exit program if unit

d757a7d

tests are run (at all) and no failures occurred (with nice message).

schveiguy force-pushed the exitafterunittest branch from 07c7267 to 8d7e951 Compare December 5, 2017 13:39

schveiguy requested a review from MartinNowak as a code owner December 5, 2017 13:39

schveiguy force-pushed the exitafterunittest branch from 8d7e951 to 59224bc Compare December 5, 2017 13:41

Update test_runner to use new design (dogfood)

6c80e31

Fix nits in PR review

schveiguy force-pushed the exitafterunittest branch from 59224bc to 6c80e31 Compare December 5, 2017 13:46

andralex merged commit 60b3d5d into dlang:master Dec 5, 2017

schveiguy deleted the exitafterunittest branch December 5, 2017 19:36

This was referenced Dec 12, 2017

Run dmd internal unittests on CIs dlang/dmd#6767

Merged

Fix windows debug build dlang/dmd#7431

Merged

wilzbach mentioned this pull request Jan 25, 2018

Fix issue 18097 - unittest symbol names can be used before semantic dlang/dmd#7761

Merged

Uh oh!

Comments

Conversation

schveiguy commented Oct 27, 2016

Uh oh!

schveiguy commented Oct 27, 2016

Uh oh!

jacob-carlborg commented Oct 27, 2016

Uh oh!

schveiguy commented Oct 27, 2016

Uh oh!

schveiguy commented Oct 27, 2016

Uh oh!

jacob-carlborg commented Oct 27, 2016

Uh oh!

schveiguy commented Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schveiguy commented Oct 27, 2016

Uh oh!

schveiguy commented Oct 27, 2016

Uh oh!

jacob-carlborg commented Oct 27, 2016

Uh oh!

schveiguy commented Oct 27, 2016

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!

deadalnix commented Oct 27, 2016

Uh oh!

schveiguy commented Oct 28, 2016

Uh oh!

wilzbach commented Dec 11, 2016

Uh oh!

schveiguy commented Dec 16, 2016

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!

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!

schveiguy commented Dec 25, 2016

Uh oh!

andralex commented Dec 25, 2016

Uh oh!

CyberShadow commented Dec 26, 2016

Uh oh!

andralex commented Dec 26, 2016

Uh oh!

schveiguy commented Dec 27, 2016

Uh oh!

schveiguy commented Dec 27, 2016

Uh oh!

jacob-carlborg Dec 27, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

schveiguy commented Oct 27, 2016 •

edited

Loading

jacob-carlborg Dec 27, 2016 •

edited

Loading