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.
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
