Add fit and fdescribe about bandit

[joakimkarlsson]

I remember toying with the idea of adding this, but decided to leave it alone because of the added complexity of first registering. I don't remember the specifics, but I decided to just go with filtering tests from the command line instead.

I'd be happy to take a look if you want to spend some time working on this, though.

/Joakim

[bfoz]

After toying with this idea myself, it looks like a dead end. The resulting code actually turned out a lot simpler and cleaner, however it seems that the C++ lambda feature isn't quite ready for this yet.

The root of the problem is that lambdas don't necessarily capture their local variables; the compiler implementation is free to create them on the stack, just like any other function's local variables. Consequently, any locals in a describe-block are potentially destroyed before any of its nested blocks are run.

Consider a simple example...

describe("when doing something", []{
    Foo myFoo;

    before_each([&]{
        // set us up the test
        myFoo.bar = 3;
    });

    it("must do something", [=]{
        Assert(myFoo.bar, Equals(3));
    });
});

In this case, describe() will run it's block immediately, whereas before_each() and it() will register their block arguments without running them. The compiler is free to optimize the describe-block into a normal function, and therefore myFoo is potentially instantiated on the call stack. After the final call to it(), the describe-block completes, the destructor for myFoo is called, and the call stack is unwound. When the test runner attempts to run the before_each block, the captured reference to myFoo is no longer valid, and undefined behavior rears its ugly head.

Implementing myFoo as a pointer, or even a unique_ptr/shared_ptr doesn't change the situation. A unique_ptr's destructor always destroys its target. Therefore, once the describe-block completes, all captures of the unique_ptr either become invalid, or point to a destroyed target. A shared_ptr won't work any better because its reference count isn't incremented when it's captured and will therefore behave like a unique_ptr in this case. I'm not sure if that's a bug, an oversight, or intentional. At the very least, it appears to be the case for clang.

A bare pointer also fails, but in two different ways. If the pointer is captured by value, the it-blocks will capture its value before the before_each block gets a change to initialize it. Therefore all of the it-blocks have invalid pointers. If the pointer is captured by reference, the it-blocks will be left holding references to a variable that was on the stack, but has since been unwound. Both failure mechanisms result in undefined behavior that has a strong tendency to be defined as a crash.

As an aside: In the process of figuring out all of this, I discovered that the above "feature" also has implications for bandit's current architecture. Because a describe-block's locals are created on the stack, they're shared between all of its nested blocks (allowing state to bleed between tests), and there's no way to ensure that the before_each block properly resets the locals between tests. In the short-term it may be sufficient to update the documentation to warn users that before_each must be considered to be a sort of constructor, and should therefore be careful to initialize everything.

Hopefully this documents the situation well enough that no one else has to go down the same rathole that I did. With a little luck, the C++ committee will someday give us the ability to force a closure to capture its local variables. Until then, or until someone can find a workaround, we seem to be stuck.

[joakimkarlsson]

Thanks for taking the time to investigate. Looks like filtering tests from
the command line will have to do for now.

/Joakim

On Mon, Apr 14, 2014 at 2:20 AM, Brandon Fosdick
[email protected]:

After toying with this idea myself, it looks like a dead end. The
resulting code actually turned out a lot simpler and cleaner, however it
seems that the C++ lambda feature isn't quite ready for this yet.

The root of the problem is that lambdas don't necessarily capture their
local variables; the compiler implementation is free to create them on the
stack, just like any other function's local variables. Consequently, any
locals in a describe-block are potentially destroyed before any of its
nested blocks are run.

Consider a simple example...

describe("when doing something", []{
Foo myFoo;

before_each([&]{
    // set us up the test
    myFoo.bar = 3;
});

it("must do something", [=]{
    Assert(myFoo.bar, Equals(3));
});

});

In this case, describe() will run it's block immediately, whereas
before_each() and it() will register their block arguments without running
them. The compiler is free to optimize the describe-block into a normal
function, and therefore myFoo is potentially instantiated on the call
stack. After the final call to it(), the describe-block completes, the
destructor for myFoo is called, and the call stack is unwound. When the
test runner attempts to run the before_each block, the captured reference
to myFoo is no longer valid, and undefined behavior rears its ugly head.

Implementing myFoo as a pointer, or even a unique_ptr/shared_ptr doesn't
change the situation. A unique_ptr's destructor always destroys its target.
Therefore, once the describe-block completes, all captures of the
unique_ptr either become invalid, or point to a destroyed target. A
shared_ptr won't work any better because its reference count isn't
incremented when it's captured and will therefore behave like a unique_ptr
in this case. I'm not sure if that's a bug, an oversight, or intentional.
At the very least, it appears to be the case for clang.

A bare pointer also fails, but in two different ways. If the pointer is
captured by value, the it-blocks will capture its value before the
before_each block gets a change to initialize it. Therefore all of the
it-blocks have invalid pointers. If the pointer is captured by reference,
the it-blocks will be left holding references to a variable that was on the
stack, but has since been unwound. Both failure mechanisms result in
undefined behavior that has a strong tendency to be defined as a crash.

As an aside: In the process of figuring out all of this, I discovered that
the above "feature" also has implications for bandit's current
architecture. Because a describe-block's locals are created on the stack,
they're shared between all of its nested blocks (allowing state to bleed
between tests), and there's no way to ensure that the before_each block
properly resets the locals between tests. In the short-term it may be
sufficient to update the documentation to warn users that before_each must
be considered to be a sort of constructor, and should therefore be careful
to initialize everything.

Hopefully this documents the situation well enough that no one else has to
go down the same rathole that I did. With a little luck, the C++ committee
will someday give us the ability to force a closure to capture its local
variables. Until then, or until someone can find a workaround, we seem to
be stuck.

Reply to this email directly or view it on GitHubhttps://github.com//issues/26#issuecomment-40324839
.

[bfoz]

No problem. I was hoping to also work in test randomizing and better isolation, but it looks like those may have to wait as well.

Although, I may have spoken too soon. I've thought of a workaround that involves running the test suite twice: once in registration mode and again in test mode. It may be an ugly hack, but it should do the trick until a better implementation becomes possible. Unfortunately it may be awhile before I can work on this again.