Semaphore mock is incomplete

[antoinealb]

Since I failed hard when discussing issue #1 I reopen the discussed issue here for more discussion.

So to make it short we need a way to emulate the case where a ressource is not available and decide how to mock this behaviour.

[pierluca]

for point 3 (count <= max_count) uc/OS-3 doesn't assert this, so tests should not IMHO.

Assert are disabled for "release" builds, but this is typically a good place for the development ones. If in the rest of the code, we release more than we take, I want to know it ASAP. fail fast, fail hard ...

idk what is the best option to emulate waiting on a ressource since all tests will (probably) run single threaded.

if we're on a single thread, semaphores don't make much sense and if count==0, we're in deadlock.

either we run multi-threaded tests or we can return a success/fail bool, so that it's the responsibility of the semaphore user to wait and try again.

[antoinealb]

Agree on the "fail fast, fail hard" part. Maybe a simple assert() is enough ? We cannot use CppUtest features such as FAIL() in C code :(

In my opinion the return bool should be for a separate function, like os_semaphore_try_take, because if we simply take the semaphore, the thread gets suspended, which is not possible to do for the user.

Running multithreaded tests is a nightmare and should be avoided I think.

[froj]

I think first we should define what we actually want to test with this mock and on what level it should behave like the real thing. The way it is right now, you can't do much more than very basic unit tests with it.

[antoinealb]

Well for last year I used such a mock to verify that my semaphores for the arm trajectory (that were used more as mutex, but that's not the point here) were acquired when needed and most important always released once the critical part is done.

If we want to test real concurrency, it might become a lot more problematic and it is not a part of unit testing in my opinion.

[pierluca]

Agree on the "fail fast, fail hard" part. Maybe a simple assert() is enough ? We cannot use CppUtest features such as FAIL() in C code :(

Yes, a simple assert seems enough to me.

In my opinion the return bool should be for a separate function, like os_semaphore_try_take, because if we simply take the semaphore, the thread gets suspended, which is not possible to do for the user.

true.

Running multithreaded tests is a nightmare and should be avoided I think.

Agreed, the tests themselves become a source of issues and too much effort is spent on them.

[froj]

I used such a mock to verify that my semaphores for the arm trajectory were acquired when needed and most important always released once the critical part is done.

Sorry, I don't understand how a simple mock like that could help you with this problem.

[antoinealb]

Consider this (trivial) example :

semaphore_t *sem;
void my_critical_function(int a)
{
    os_semaphore_take(sem);

    // error occurs
    if (a < 0) {
        // ooops i forgot to release semaphore
        return;
    }

    os_semaphore_release(sem);
}

By writing a test for the case where a < 0 (which I did during development, because TDD), and this test checks for correct semaphore release, you would have caught the bug, no ?

[antoinealb]

Example from real codebase of Debra:

TEST(ArmTestGroup, ArmShutdownIsAtomic)
{
    arm_shutdown(&arm);
    CHECK_EQUAL(1, arm.trajectory_semaphore.acquired_count);
    CHECK_EQUAL(1, arm.trajectory_semaphore.count);
}

[froj]

Ok, I see. Still need to get my head around TDD.

[antoinealb]

Now, you could even do something in your teardown method like :

TEST_GROUP(ArmTestGroup)
{
    arm_t arm;
    void teardown()
    {
        CHECK(arm.trajectory_semaphore.count == arm.trajectory_semaphore.max_count);
    }
};

So now every test you wrote for weird conditional paths also checks that you correctly released the semaphore. Pretty cool huh ? :D

[froj]

Now, if we consider that the tests are done purely single-threaded, we can also fail when the resource is unavailable.

[antoinealb]

You mean in the real application ? Of course, but if you use take, then your thread gets suspended until that ressource is available again.

[froj]

I mean when we do the unit tests and you're trying to take a semaphore that is 0, then the test has to fail.

[pierluca]

it's counter-intuitive to me because you might think of tests where that behaviour is desired, but I think that within the single-threaded constraint, you're absolutely right.

[antoinealb]

I also think it is valid application behaviour, for example if you use the semaphore as a free queue slots counter for example, it is perfectly valid for the application to try to access the queue when no slots are available, but I don't see any work-around.

I will add an assert for this in a new pull request (can anyone merge #3 please ?).

[antoinealb]

I implemented this behaviour in PR #4 . Once this gets merged, I think we can close this issue, or is there any points still not fixed ?

[froj]

Maybe we should open a new issue about what other function(alitie)s should be exported.

[antoinealb]

Totally agree, open new issues for other functionalities.