[PATCH 00/33] Test framework improvements

[Sebastian Huber]

On 22/07/2020 07:04, Chris Johns wrote:

> On 22/7/20 1:04 am, Sebastian Huber wrote:
>> This patch set adds a couple of improvements to the test framework:
>>
>> * The header file changes from <t.h> to <rtems/test.h>.
>>
>> * Support for a stack of test fixtures.  This helps to write test
>>    building blocks.
>>
>> * The test check messages are now optional.
>>
>> * The support for interrupt tests and change all the interrupt critical
>>    tests to use this new test support.  This should address the sporadic
>>    failures and timeouts.
>>
>> For the documentation please have a look at:
>>
>> https://ftp.rtems.org/pub/rtems/people/sebh/eng2.pdf
>>
> I assume you asking us to look only at section 8?
Yes, and actually only the new sections corresponding to the patch.
>
> The doco looks good. The following are some review comment:
A full review is also nice ;-)
>
> 8.1
>
> a) Supported languages?
I added "Tests can be written in C or C++".
>
> 8.1.1
>
> a) The wording ".. outcome all right, ..." seems non-specific for someting I
> would assume needs to be specific. What about " ... is not the required outcome
> for the test, ..."?
I tried to clarify the wording a bit.
>
> b) As previously discussed there are a few more states a test result can be
> other than pass and fail and I thikn the wording here may need tightening. I am
> not sure what is needed. Also how do resource leaks effect the result?

Currently, the test framework supports only passed and failed at the 
level of a test check. I am not sure if adding more states at this level 
is really helpful.

>
> 8.1.2
>
> a) It test case naming to be specified or can be use anything we like, ie lower
> case with `_`, Camelcase, ...?
It should be CamelCase, but this is a coding style issue which should 
not be addressed in this part of the documentation.
>
> b) Change "static constructors" to "static C constructors".
Ok.
>
> 8.1.3
>
> a) Change "test case as well as to give thescope for log messags." to " test
> case as well as the scope for log messages."
Ok.
>
> b) Change "function. It can be set within the scope of one .." to "function and
> can be set within the scope of one .."
I reworded it a bit based on Gedare's comment.
>
> c) Does the framework provide a standard way to export a dynamic test
> environment so the test and the results can be reviewed as a complete set of data?
Sorrry, I don't understand this question.
>
> 8.1.4
>
> a) Should "Each non-quiet test check fetches.." be "A non-quiet test check
> fetches.."?
Ok.
>
> 8.1.5
>
> a) Change "..if various resources are leaked .." to ".. if various resources
> have leaked ..".
Ok.
> 8.1.7
>
> a) Change "You can add test case destructors with T_add_destructor(). They
> are.." to "A test case destructor can be added with T_add_destructor(). The
> destructors are..".
I kept the "You can" style for now.
>
> b) Maybe add it is best to use statically allocated memory only?
Ok.
>
> 8.1.8
>
> a) "Meet its expecation" ... I am not sure what this means, who expecation and
> what does expecation mean? Do you mean " .. test check meets the check's
> exepected outcome."?
>
> b) I am not sure what "The actual value ..." is referring to. The value the test
> expects to be the input from the test to the check? Ah coming back to here, I
> see this is in the next bit. Would adding a little bit in the text before using
> the special words like actual and expected be helpful?
I changed the wording a bit.
>
> c) I think the T_assert_ etc variants of the test checks should be explained
> somewhere. I think it is important to know tests can continue even after a fail
> or they can be made to stop. I had noted I had not seen this documented and I
> have now found it buried in here.
It split up the "Test Check Variant Conventions" and added a "Test Check 
Type Conventions".
>
> d) I think the word `available` can be removed from the initial sentence on all
> sub-sectons?
This would need a bit of rewording .
>
> 8.1.8.10
>
> a) I think type variant piece of test should be moved to the top and add to the
> initial sentence so you have " are available where the type variant `xyz` must
> be ...".
Ok.
>
> 8.1.8.12
>
> a) What about 0 or EINTR as OK? :)
>
> 8.1.6
>
> a) Are the printf variants check for type correctness by the compiler? If so I
> suggest this is mentioned.
No, but I think this should be added.
>
> 8.1.11
>
> a) Can "You can .." be removed so it is not person specific? For example "You
> can convert time into ticks with the" ciuld be "The time can be converted into
> ticks with the"? This comment covers all the "You can"s. :)
I will keep this you can style for now.
>
> 8.1.13
>
> a) What is the picture drawn in?
Libreoffice Draw.
>
> I will comment on the patches as well.
Thanks for the review.