Re: Mynewt test facilities

[Christopher Collins]

Thanks, Will.

Responses to inline comments inline :).

On Sun, Mar 03, 2019 at 06:18:46PM -0800, will sanfilippo wrote:
> > ### PROPOSALS
> > 
> > 1. (testutil): Modify `TEST_CASE()` to call
> > `sysinit()` if `MYNEWT_VAL(SELFTEST)` is enabled.
> Would we want all TEST_CASE() to call sysinit if a single syscfg is
> defined? Maybe for some tests you want it and some you dont?

Before I address this, I want to clarify my proposal a bit.  I did a
poor job of setting up the context, so I'll try again here.  Also, you
would get tired of seeing "IMHO" at the end of every sentence, so I
won't write it.  Please just understand that all the below is just my
opinion :).

Test cases should be as self-contained as possible.  We don't want any
restrictions on how test cases are ordered.  For example, something like
this is bad (hypothetical):

/* Test basic FCB functionality. */
test_case_fcb_basic();

/* The above test initialized and populated the FCB.  Make sure we
 * can delete an entry from it.
 */
test_case_fcb_delete();

The ordering of these test cases matters.  The second case depends on
side effects from the first case.  These kinds of tests are difficult to
maintain, and often it is difficult to tell what is even being tested.

Every test case should start from a clean state.  If
`test_case_fcb_delete()` needs a pre-populated FCB, it should start by
initializing and populating an FCB with the exact contents it needs.

This is why it is good to execute `sysinit()` at the start of each test
case - it lets us clean up state left over from prior test cases.  This
eliminates a lot of manual clean up code, and it helps ensure
correctness of old tests as new ones are added.

Ideally we could just call `sysinit()` at the start of each test case,
whether the test is simulated or performed on real hardware.
Unfortunately, we can't call `sysinit()` in hw tests.  The problem is
that hw tests specifically need to maintain some state between test
cases.  Calling `sysinit()` in a hw test case would reset the runtest
package, reinitialize the test result logs, and otherwise make the
system unusable as a test server.

So I think we should call `sysinit()` at the start of a test case
whenever possible.  In other words, call `sysinit()` for every self
test.  I don't think we should make this behavior conditional on a
syscfg setting, but as I said, I am totally open to opposing viewpoints.

I do think you raise a good point, though.  The semantics of
`TEST_CASE()` should not differ between self tests and hw tests.
Instead, we should create a new macro which creates a test case and
calls `sysinit()` at the top.

That is:

`TEST_CASE()` - creates a test case that does NOT call `sysinit()`.
`TEST_CASE_SELF()` - creates a test case that DOES call `sysinit()`.

(macro name off the top of my head.)

Self tests would use `TEST_CASE_SELF()`; hw tests would use
`TEST_CASE()`.

At first, I thought it would be confusing to have two macros.  Now I
think it is even more confusing to have one macro with varying
semantics.

I have some more changes I would like to make to the testutil API, but I
will save that for a future email (or PR).

> > This example illustrates a few requirements of taskpool:
> > 
> >1) A taskpool task (tp-task) is allowed to "run off" the end of its
> >   handler function.  Normally, it is a fatal error if a Mynewt
> >   task handler returns.
> > 
> Not sure why I do not like this but is there some need to have a task
> basically return?

It is not strictly needed, but it is a convenient way for a task to
signal that it has completed.  Otherwise, each of these temporary tasks
would need to set some condition variable and keep looping.  That isn't
terribly complicated, but I do think it is more complicated than this
needs to be.  These tasks are not meant to loop endlessly like most, so
why require them to be written that way?

Just to clarify - I am not proposing a change to the kernel scheduler.
The handlers for these tp-tasks would just be wrapped with something
that provides this special behavior.

> Seem fairly reasonable. Is taskpool_create() designed to save the test
> developer some time creating tasks or does it have some other
> functionality?

Its main purpose is to allow tasks to be reused among a set of test
cases.  This isn't so important for self tests, but in hw tests, it is
wasteful for each test case to allocate its own pool of tasks and
stacks.  Since only one test runs at a time, these test cases can share
a pool of tasks.

Thanks,
Chris

Re: Mynewt test facilities

[will sanfilippo]

Some comments inline. 

> On Feb 27, 2019, at 6:08 PM, Christopher Collins  wrote:
> 
> Hello all,
> 
> In this email, I would like to discuss Mynewt's test facilities.
> 
> ### INTRO (AKA TL;DR)
> 
> Unit tests come in two flavors: 1) simulated, and 2) real hardware.
> Writing a test capable of running in both environments is wasted
> effort.  Let's revisit Mynewt's test facilities with a clear
> understanding that they must solve two different use cases.
> 
> In this email, I want to:
>1. Clarify the differences between the two test environments.
>2. Propose some high-level changes to Mynewt's test facilities.
> 
> ### TEST ENVIRONMENTS
> 
> The two test environments are summarized below.
> 
> # SIMULATED
> 
> Simulated tests ("self tests"):
>* Are executed with `newt test`.
>* Automatically execute a set of tests.  No user intervention
>  required.
>* Report results via stdout/stderr and exit status.
>* Terminate when the tests are complete.
> 
> A typical self test is a very basic application.  Usually, a self test
> contains the package-under-test, the testutil package, and not much
> else.  Most self tests don't even include the OS scheduler; they are
> simply a single-threaded sequence of test cases.  For these simple
> tests, `sysinit()` can be executed between each test case, obviating
> the need for "setup" and "teardown" code.  Self tests are allowed to
> crash; such crashes get reported as test failures.  Finally, self tests
> have effectively unlimited memory at their disposal.
> 
> # REAL HARDWARE
> 
> Test apps that run on real hardware ("hw tests"):
>* Run a test server.  The user executes tests on demand with the
>  newtmgr `run` command.
>* Typically contain test suites from several packages.
>* Record results to a Mynewt log.
>* Run indefinitely.  The device does not reset itself between
>  tests.
> 
> Hw tests are more complex than self tests.  These test apps need to
> include the scheduler, the newtmgr server, a transport (e.g., BLE), and
> a BSP and all its drivers.  Test code cannot assume a clean state at
> the start of each test.  Instead, the code must perform manual clean up
> after each test case.  Hw test cases must be idempotent and they must
> not crash.  Finally, memory is constrained in this environment.
> 
> # PREFER SELF
> 
> Clearly, self tests are easier to write than hw tests.  Equally
> important, self tests are easier to run - they can run in automated CI
> systems without the need for a complicated test setup.
> 
> So self tests should always be preferred over hw tests.  Hw tests should
> only be used when necessary, i.e., when testing drivers or the system
> as a whole.  I won't go so far as to say there is never a reason to run
> the same test in both environments, but it is so rarely needed that it
> is OK if it requires some extra effort from the developer.
> 
> ### TEST FACILITIES
> 
> Mynewt has two unit test libraries: `testutil` and `runtest`.
> 
> # TESTUTIL
> 
> Testutil is a primitive unit test framework.  There really isn't
> anything novel about this library - suites, cases, pass, fail, you get
> the idea :).  This library is used in both test environments.
> 
> I have one concern about this library:
> 
>In self tests, does `sysinit()` get called automatically at the
>start of each test case?
> 
> Currently, the answer is no.
> 
> # RUNTEST
> 
> Runtest is a grab bag of "other" test functionality:
> 
>1. Command handler for the `run` newtmgr command.
>2. API for logging test results.
>3. Generic task creation facility for multithreaded tests.
> 
> Features 1 and 2 are only needed by hw tests.  Feature 3 is needed by
> both self tests and hw tests.
> 
> ### PROPOSALS
> 
> 1. (testutil): Modify `TEST_CASE()` to call
> `sysinit()` if `MYNEWT_VAL(SELFTEST)` is enabled.
Would we want all TEST_CASE() to call sysinit if a single syscfg is defined? 
Maybe for some tests you want it and some you dont?

> 
> 2. (runtest): Remove the task-creation functionality from `runtest`.
> This functionality can go in a new library (call it "taskpool" for the
> sake of discusion).
> 
> 3. (taskpool) Further, I think the taskpool functionality could be
> tailored specifically towards unit tests.  The remainder of this email
> deals with this proposal
> 
> I will use an example as motivation for this library.  Here is a simple
> test which uses our to-be-defined taskpool library.  This example tests
> that a `cbmem` can handle multiple tasks writing to it at the same
> time.  Note: this is not meant to be good test code :).
> 
>static struct cbmem cbm;
> 
>/**
> * Low-priority task handler.  Writes 20 entries to the cbmem.
> * Uses a rate of 1 OS tick per write.
> */
>static void
>task_lo(void *arg)
>{
>int i;
> 
>for (i = 0; i < 20; i++) {
>cbmem_append(, "hello from task_lo", 18);
>os_time_delay(1);
>}
>