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
