Add enhanced unittest support #3518

jpf91 · 2014-05-01T18:58:55Z

As discussed in the newsgroup this reboots dlang/druntime#308
and #1131

Ping @andralex
This provides enough to run unittests separately and in different threads, no support for naming tests yet. @andralex what's the preferred syntax for that?

Detailed description

The new information is available from the unitTests member of every ModuleInfo.
unitTests is an array of __UnitTest structs. The __UnitTest struct provides the unittest function, location, etc.

Example

bool tester()
{
    import std.stdio;
    //iterate all modules
    foreach(info; ModuleInfo)
    {
        //iterate unit test in modules
        foreach(test; info.unitTests)
        {
            //access unittest information
            writefln("ver=%s file=%s:%s disabled=%s func=%s", test.ver, test.file, test.line, test.disabled, test.func);
            //execute unittest
            test.func();
        }
    }
    return true;
}

shared static this()
{
    Runtime.moduleUnitTester = &tester;
}

Example test runners

Simple example: http://dpaste.dzfl.pl/56697348500c
Output: http://dpaste.dzfl.pl/c0c706b0cea4

Parallel runner: http://dpaste.dzfl.pl/cef0a42936d6
Output: http://dpaste.dzfl.pl/d2445d87a9d7

Available information

The following information is emitted for every unittest:

void function() func. Call it, pass it to another thread, disassemble, ...
string file: file name. nice for IDE cause not all unittests always belong to the module (for example, importing std.stdio imports two unittests from std.format)
uint line: line of test declaration. For IDE
bool disabled true if test is marked @disable. (This can be removed if it's controversial)

yebblies · 2014-05-02T06:40:11Z

This places the unittest array before the module name in the module info. I'd like some feedback from DMD devs if this is an issue. If we place it after the name we have a problem with alignment:
The module name may have an arbitrary length and dtsize_t doesn't align the output. No problem on X86, no-go for older ARMs. So if someone tells me how to get dtsize_t to align the output I could also add this after the module name.

Easiest way is probably to pad the module name to the target word size.

jpf91 · 2014-05-02T07:45:32Z

Thanks @yebblies , I've updated the code to pad the module name as you suggested.

ghost · 2014-05-02T22:23:10Z

src/toobj.c

@@ -182,8 +185,17 @@ void Module::genmoduleinfo()
        const char *name = toPrettyChars();
        namelen = strlen(name);
        dtnbytes(&dt, namelen + 1, name);
+        if((namelen + 1) % Target::ptrsize != 0)


Can you add a comment on what's going on here? Too much magic to my eyes.

Sure. It's padding the length to a multiple of Target::ptrsize.

WalterBright · 2014-05-06T19:49:46Z

Ah, I see this is rebooted as dlang/druntime#782

jpf91 · 2014-05-07T14:00:45Z

Added named unittests. @WalterBright do Identifiers have some maximum length? IIRC @andralex wanted the name to be part of the mangled symbol name. That code generates names using Lexer::uniqueId which used a static buffer though. I guess I can just change Lexer::uniqueId to use OutBuffer?

andralex · 2014-05-07T15:19:14Z

I get the intended use of @disable but I don't think locating it with the unittest is the right approach. The test runner could and should do its own bookkeeping with what tests should be ran, skipped, tried once in a while etc.

andralex · 2014-05-07T15:20:20Z

Is V1/V2 about D1/D2? Then I guess we shouldn't complicate things with D1 support.

WalterBright · 2014-05-09T19:08:52Z

src/func.c

    return Lexer::uniqueId(buf.peekString());
 }

-UnitTestDeclaration::UnitTestDeclaration(Loc loc, Loc endloc, char *codedoc)
-    : FuncDeclaration(loc, endloc, unitTestId(loc), STCundefined, NULL)
+UnitTestDeclaration::UnitTestDeclaration(Loc loc, Loc endloc, char *codedoc, const char *name)


If a name is supplied, that name should be used. unitTestId should only be used if a name is not supplied.

I'm sorry, but I can't read your mind. If you have such detailed beliefs how this should be implemented you could tell me and at least give detailed specifications instead of one line answers before I implement this stuff. This would save me and you lots of time. Or you could have written these 10 lines of code and pasted them here, or file a pull request against my dmd fork....

For example unitTestId should only be used if a name is not supplied. does not make sense to me. This immediately opens lots of questions:

Should unittest identfiers conflict with other identifiers in the same module (functions, variables, ...)

How should named test be mangled then? Using unitTestId but not using uniqueId would make sense, but not using unitTestId? Than named /unnamed test have completely different mangles?

What does that name should be used mean? You don't want the line in the mangled name?

So please, give exact mangle strings for the following examples:

unittest NamedTest {} unittest {} unittest {}

jpf91 · 2014-05-10T07:45:14Z

Seems like some comments were lost when rebasing:
@Dicebot

Defining test name as identifier limits selection of nicely formatted names. Why not
unittest "My Named Unit Test" {?

That was not my decision, you have to ask @WalterBright about that.

MartinNowak · 2014-05-20T17:39:16Z

src/glue.c

+        {
+            if (!StructDeclaration::UnitTest)
+            {
+                warning(loc, "mismatch between compiler and object.d or object.di found. "


We have a special Funktion for that, named something like ObjectWarnIfMissing.

Are you sure? I grepped for object.d but only found ObjectNotFound which errors and is for typeinfo afaik. So should we make this an error?

It is used exactly for this purpose (see here)

The new information is available from the 'unitTests' member of every ModuleInfo. 'unitTests' is an array of __UnitTest structs. The __UnitTest struct provides the unittest function, location etc.

jpf91 · 2014-05-25T09:35:42Z

Rebased.

Restored optimization in lexer.c
Use identifier instead of char*
Now named as:

//unnamed tests (unchanged)
__unittestL{line}_{unique id}
//named tests (N for NAME)
__unittestL{LINE}_N{identifier}

example:
unittest{} => _D4test14__unittestL1_1FZv
unittest testA {} => _D4test20__unittestL6_N5testAFZv

The druntime pull was pulled with the disabled field, so I left it in here as well. I think it is useful, but it's a small language change (@disabled unittest is no longer ignored) so I can remove this if desired. I'd have to file a new druntime pull then, though.

MartinNowak · 2014-05-25T14:02:17Z

The test runner could and should do its own bookkeeping with what tests should be ran, skipped, tried once in a while etc.

How would the test runner know such things from a function pointer?

MartinNowak · 2014-05-25T14:22:52Z

I think adding some more meta information to unittests makes a lot of sense. Using @disabled to disable runningof a test is superior to disable compiling them with version (none), so we should support it. Location information is very useful for tooling.
So unless there is some constructive criticism I will merge this soon.

MartinNowak · 2014-05-25T14:31:05Z

There should be a test case for the parser addition, we'll need an update of the grammar, the specification should mention @disable and named tests and this needs to go into the changelog.

MartinNowak · 2014-05-26T02:08:26Z

Are we (@WalterBright) sure about using an identifier? It makes it look like a symbol although it's just runtime information and not possible to reference the test.
IMHO using a string (disallowing umangleable characters) makes for a cleaner syntax.

unittest("my_test_for_foobar")
{
}

vs.

unittest my_test_for_foobar
{
}

WalterBright · 2014-06-01T04:28:58Z

@MartinNowak what is the rationale for @disable ? Why not make it a symbol accessible via the symbolic debug info? How would you set a breakpoint on a random string?

I disagree with all this meta information, as I've pointed out before. There is no gain from essentially duplicating symbolic debug info.

MartinNowak · 2014-06-01T15:31:50Z

@MartinNowak what is the rationale for @disable ?

A disabled test (which is currently failing) can still be listed in the output. Also @disabled tests are still compiled, so they are semantically checked.

Why not make it a symbol accessible via the symbolic debug info? How would you set a breakpoint on a random string?

That makes sense, the syntax still looks a little weird to me.

I disagree with all this meta information, as I've pointed out before. There is no gain from essentially duplicating symbolic debug info.

It's very little information and symbolic debug info is not accessible to a test runner. Being able to know the location of a test is useful for tooling (like IDEs).

WalterBright · 2014-06-01T18:57:13Z

A disabled test (which is currently failing) can still be listed in the output.

Since failing a unittest no longer stops the rest of the unittests from running, this doesn't seem to add much value.

Also @disabled tests are still compiled, so they are semantically checked.

Also seems of little value. (Note that version(none) tests will still be grammar checked, though not semantically.)

Being able to know the location of a test is useful for tooling (like IDEs).

Having individual names for the unittests makes them trivially greppable. The module name is also available via the ModuleInfo, no need to duplicate it.

MartinNowak · 2014-06-01T21:47:05Z

Since failing a unittest no longer stops the rest of the unittests from running, this doesn't seem to add much value.

Agreed, it's only a marginal improvement.

Having individual names for the unittests makes them trivially greppable.

But most tests aren't named and likely never will be named.

The module name is also available via the ModuleInfo, no need to duplicate it.

Yes, but the module name is not the file name. It's difficult to map module names back to files when you have multiple import pathes.
What are we optimizing here for? Binary size? Doesn't make sense in the context of unittests.
As compromise was could stuff the filename and the linenumber into the unittest name for unnamed tests.

joakim-noah · 2014-06-07T23:40:40Z

Since failing a unittest no longer stops the rest of the unittests from running, this doesn't seem to add much value.

Does this actually work? I tried applying this pull to dmd/Linux_32 and running the druntime and phobos tests: the tests stop whenever a single test fails.

@jpf91, I notice that in your simple example above, the failing test is the last one, so maybe that aspect needs more work, likely in druntime?

jpf91 · 2014-06-08T17:14:57Z

@joakim-noah This wasn't added to the druntime test runner as it is a user visible change and when I initially proposed this people were completely against changing any user facing output. (Even printing 'pass' for passing modules was considered unacceptable. Ironically we now print passing modules.)

All:
It's become obvious that we won't find an agreement here and I don't want to waste more time on this. dlang/druntime#832 reverts the already pulled druntime changes. I'll keep the dmd code available at https://github.com/jpf91/dmd/tree/newUnittest and the druntime code at https://github.com/jpf91/druntime/tree/newUnittest
If anybody wants to work on this feature consider that code public domain and uncopyrighted (or whatever legal stuff is necessary to reuse that code).

MartinNowak · 2014-06-08T19:20:34Z

It's become obvious that we won't find an agreement here

I think we're pretty close, @disable is out, identifiers/symbols are used for debugging, instead of the filename we can use ModuleInfo.name and the line number can be put into the unittest name field for anonymous tests.

Add OpenBSD backtrace support

jpf91 mentioned this pull request May 1, 2014

Enhanced unittest support dlang/druntime#782

Merged

ghost reviewed May 2, 2014
View reviewed changes

WalterBright reviewed May 9, 2014
View reviewed changes

MartinNowak reviewed May 20, 2014
View reviewed changes

jpf91 added 2 commits May 25, 2014 11:21

Add enhanced unittest support

0224cfe

The new information is available from the 'unitTests' member of every ModuleInfo. 'unitTests' is an array of __UnitTest structs. The __UnitTest struct provides the unittest function, location etc.

Support named unittests

b25e090

MartinNowak closed this Jun 1, 2014

MartinNowak reopened this Jun 1, 2014

jpf91 closed this Jun 8, 2014

joakim-noah mentioned this pull request Jun 12, 2014

Revert new unittest changes in druntime dlang/druntime#832

Merged

ibuclaw pushed a commit to ibuclaw/dmd that referenced this pull request Jul 10, 2022

Merge pull request dlang#3518 from ibara/openbsd-execinfo

f964462

Add OpenBSD backtrace support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add enhanced unittest support #3518

Add enhanced unittest support #3518

jpf91 commented May 1, 2014

yebblies commented May 2, 2014

jpf91 commented May 2, 2014

ghost May 2, 2014

jpf91 May 3, 2014

WalterBright commented May 6, 2014

jpf91 commented May 7, 2014

andralex commented May 7, 2014

andralex commented May 7, 2014

WalterBright May 9, 2014

jpf91 May 10, 2014

jpf91 commented May 10, 2014

MartinNowak May 20, 2014

jpf91 May 25, 2014

MartinNowak May 25, 2014

jpf91 commented May 25, 2014

MartinNowak commented May 25, 2014

MartinNowak commented May 25, 2014

MartinNowak commented May 25, 2014

MartinNowak commented May 26, 2014

WalterBright commented Jun 1, 2014

MartinNowak commented Jun 1, 2014

WalterBright commented Jun 1, 2014

MartinNowak commented Jun 1, 2014

joakim-noah commented Jun 7, 2014

jpf91 commented Jun 8, 2014

MartinNowak commented Jun 8, 2014

Add enhanced unittest support #3518

Add enhanced unittest support #3518

Conversation

jpf91 commented May 1, 2014

Detailed description

Example

Example test runners

Available information

yebblies commented May 2, 2014

jpf91 commented May 2, 2014

ghost May 2, 2014

Choose a reason for hiding this comment

jpf91 May 3, 2014

Choose a reason for hiding this comment

WalterBright commented May 6, 2014

jpf91 commented May 7, 2014

andralex commented May 7, 2014

andralex commented May 7, 2014

WalterBright May 9, 2014

Choose a reason for hiding this comment

jpf91 May 10, 2014

Choose a reason for hiding this comment

jpf91 commented May 10, 2014

MartinNowak May 20, 2014

Choose a reason for hiding this comment

jpf91 May 25, 2014

Choose a reason for hiding this comment

MartinNowak May 25, 2014

Choose a reason for hiding this comment

jpf91 commented May 25, 2014

MartinNowak commented May 25, 2014

MartinNowak commented May 25, 2014

MartinNowak commented May 25, 2014

MartinNowak commented May 26, 2014

WalterBright commented Jun 1, 2014

MartinNowak commented Jun 1, 2014

WalterBright commented Jun 1, 2014

MartinNowak commented Jun 1, 2014

joakim-noah commented Jun 7, 2014

jpf91 commented Jun 8, 2014

MartinNowak commented Jun 8, 2014