Fix #982, Add test for object id inline functions #990

zanzaben · 2021-05-07T15:19:49Z

Describe the contribution
Fixes #982
Adds test for inline object id functions

Testing performed
Build and run unit test

Expected behavior changes
No impact to behavior

System(s) tested on
Ubuntu 20.04

Contributor Info - All information REQUIRED for consideration of pull request
Alex Campbell GSFC

jphickey

Some comments noted inline.

I would also suggest just doing one record of each type. "maxing it out" by calling OS_ObjectIdAllocateNew until it fails seems like overdoing it. These inline functions are just simple ops and generally have no conditionals, there isn't a real benefit to testing it with every possible value.

src/unit-test-coverage/shared/src/coveragetest-idmap.c

skliper · 2021-05-12T14:39:34Z

I prefer maxing out for the comparison test. Would catch any strange rollover bugs, or bug in creating the unique IDs. This isn't a huge number, so seems reasonable for the additional "black box" coverage to ensure uniqueness.

jphickey · 2021-05-12T14:51:32Z

I prefer maxing out for the comparison test. Would catch any strange rollover bugs, or bug in creating the unique IDs. This isn't a huge number, so seems reasonable for the additional "black box" coverage to ensure uniqueness.

That's done by the existing test cases for OS_ObjectIdAllocateNew though - this calls it OS_OBJECT_INDEX_MASK times (plus some extra) to ensure the rollover happens correctly.

If the objective of these tests should be to check the inline functions, just a few choice values should be enough (I'd consider adding OS_OBJECT_ID_UNDEFINED too, which is not part of the current set). They are really just type-safety tools at the end of the day (typecast replacement and a type-specific equality check) and don't have any real logic in them.

skliper · 2021-05-12T15:09:12Z

I still don't see any harm to confirm all the ID's across all the types are never equal except to themselves. It adds very little complexity to the test. The test for equality after the conversion from/to index across all IDs that can be allocated also shows the conversion doesn't change anything or cause loss of info. Why skip when there's such little "cost"? Plus it's already implemented... so is it really worth backing out a more thorough test? I don't really care that it may be caught by some other test somewhere based on how it's implemented, that other test doesn't also exercise the conversions so I'd rather keep the full range i here. Also avoids dependencies on other tests (say the other test changes, could make a reduced test here now miss something else).

astrogeco · 2021-05-12T16:20:34Z

CCB:2021-05-12 CHANGES REQUESTED

Don't like repeating things in tests
Open issue to move API to header? This can help to expose for the coverage test.
Add a test against "undefined".
The problem is that we don't test these in functional tests
If time is trivial, it's okay to overtest a little
Watch out for log growing too big in the future or tests taking too long

astrogeco · 2021-05-12T18:21:01Z

@zanzaben do we want to change this to draft since there more work to do?

zanzaben · 2021-05-13T14:31:15Z

All requested changes have been made.

jphickey

Getting better, but still a few more concerns.

src/unit-test-coverage/shared/src/coveragetest-idmap.c

jphickey · 2021-05-13T16:00:53Z

src/unit-test-coverage/shared/src/coveragetest-idmap.c

+                recordscount++;
+            }
+
+        } while (actual == OS_SUCCESS);


The other thing I'd like to see as part of this loop is a check that recordscount does not exceed OS_MAX_TOTAL_RECORDS. It shouldn't when things are working right.... but should confirm that.

src/unit-test-coverage/shared/src/coveragetest-idmap.c

jphickey · 2021-05-14T14:56:33Z

I merged this into my branch and ran it, looks OK ....

BUT - I still think we should reconsider testing all the values. In my test this produced 79243 test cases, which all get written to the log file.

Yes, it runs pretty fast on Linux where the console is local. But the log file ends up being almost 6MB in size. If running this on VxWorks on a 9600 baud serial connection, this will likely take 1-2 hours to print all those messages to the console.

IMO no test should produce more than 1000 cases (there are 3 digits allocated in the template). Even that is a little excessive.

Can we just limit it to 5 IDs of each type?

src/unit-test-coverage/shared/src/coveragetest-idmap.c

jphickey · 2021-05-14T15:20:10Z

Alternatively - if we can switch the "does not equal" test (noted in my previous comment) to only assert if it fails. In other words, something like

if (ObjectIdEqual(...)) 
{
    UtAssert_Failed(....)
}

This cuts the number of logged test cases from nearly 80k to about 580.

astrogeco · 2021-05-18T12:42:45Z

@zanzaben and @jphickey are we good to go with this one?

jphickey · 2021-05-18T15:28:39Z

@zanzaben and @jphickey are we good to go with this one?

Yes, I think its all set.

nasa/cFE#1482, Resolve sequence count auto-increment rollover bug nasa/osal#985, rename hooks to handlers nasa/osal#1000, propagate status code in OS_rmdir nasa/osal#1001, rework "unit-tests" to use macros nasa/osal#1003, remove extra newlines in utassert logs nasa/osal#990, Add test for object id inline functions nasa/osal#998, fixed invalid inputs for OS_mkdir nasa/osal#812, Improves config guide documentation

Combines: nasa/cFE#1508, cFE v6.8.0-rc1+dev580 nasa/osal#1006, osal v5.1.0-rc1+dev452 Includes: nasa/cFE#1482, Resolve sequence count auto-increment rollover bug nasa/cFE#1491, Correctly format code block section terminator nasa/cFE#1530, Fix typos in developer guide nasa/osal#985, rename hooks to handlers nasa/osal#1000, propagate status code in OS_rmdir nasa/osal#1001, rework "unit-tests" to use macros nasa/osal#1003, remove extra newlines in utassert logs nasa/osal#990, Add test for object id inline functions nasa/osal#998, fixed invalid inputs for OS_mkdir nasa/osal#812, Improves config guide documentation nasa/osal#987, Show CodeQL Preview

Combines: nasa/cFE#1508, cFE v6.8.0-rc1+dev580 nasa/osal#1006, osal v5.1.0-rc1+dev452 Includes: nasa/cFE#1482, Resolve sequence count auto-increment rollover bug nasa/cFE#1491, Correctly format code block section terminator nasa/cFE#1530, Fix typos in developer guide nasa/osal#985, rename hooks to handlers nasa/osal#1000, propagate status code in OS_rmdir nasa/osal#1001, rework "unit-tests" to use macros nasa/osal#1003, remove extra newlines in utassert logs nasa/osal#990, Add test for object id inline functions nasa/osal#998, fixed invalid inputs for OS_mkdir nasa/osal#812, Improves config guide documentation nasa/osal#987, Show CodeQL Preview Co-Authored-By: Jake Hageman <skliper@users.noreply.github.com> Co-Authored-By: Joseph Hickey <joseph.p.hickey@nasa.gov> Co-Authored-By: Ariel Adams <ArielSAdamsNASA@users.noreply.github.com> Co-Authored-By: Alex Campbell <zanzaben@users.noreply.github.com> Co-Authored-By: Tobias Nießen <tniessen@users.noreply.github.com> Co-Authored-By: Jonathan Bohren <jbohren-hbr@users.noreply.github.com> Co-Authored-By: Andrei Tumbar <Kronos3@users.noreply.github.com>

Fix nasa#988, add casts on printf calls

zanzaben added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label May 10, 2021

zanzaben force-pushed the fix982_object_id_test branch from 286dc92 to dbd78d3 Compare May 10, 2021 14:48

jphickey requested changes May 12, 2021

View reviewed changes

zanzaben force-pushed the fix982_object_id_test branch from dbd78d3 to cf3b645 Compare May 12, 2021 15:08

astrogeco removed the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label May 12, 2021

zanzaben marked this pull request as draft May 12, 2021 18:29

zanzaben changed the title ~~Fix #982, Add test for object id inline functions~~ WIP Fix #982, Add test for object id inline functions May 12, 2021

zanzaben force-pushed the fix982_object_id_test branch 2 times, most recently from 41281d0 to 9191298 Compare May 13, 2021 14:24

zanzaben changed the title ~~WIP Fix #982, Add test for object id inline functions~~ Fix #982, Add test for object id inline functions May 13, 2021

zanzaben marked this pull request as ready for review May 13, 2021 14:30

jphickey requested changes May 13, 2021

View reviewed changes

Fix nasa#982, Add test for object id inline functions

3ec2c2e

zanzaben force-pushed the fix982_object_id_test branch from 9191298 to 3ec2c2e Compare May 14, 2021 13:48

jphickey approved these changes May 14, 2021

View reviewed changes

jphickey reviewed May 14, 2021

View reviewed changes

src/unit-test-coverage/shared/src/coveragetest-idmap.c Outdated Show resolved Hide resolved

jphickey force-pushed the fix982_object_id_test branch from f4e02e2 to b9be5b0 Compare May 14, 2021 17:16

Update nasa#982, Only log failures in id inequality check

b18dcc6

This cuts the number of logged test cases from nearly 80k to about 580.

jphickey force-pushed the fix982_object_id_test branch from b9be5b0 to b18dcc6 Compare May 14, 2021 17:19

astrogeco added the CCB:Approved Indicates code review and approval by community CCB label May 16, 2021

astrogeco changed the base branch from main to integration-candidate May 18, 2021 12:42

astrogeco merged commit 278749a into nasa:integration-candidate May 18, 2021

astrogeco added the IC:2021-05-18 label May 18, 2021

astrogeco mentioned this pull request May 19, 2021

osal Integration candidate: 2021-05-18 #1006

Merged

astrogeco mentioned this pull request May 19, 2021

cFS Bundle Integration Candidate:2021-05-18 nasa/cFS#259

Merged

skliper added this to the 6.0.0 milestone Sep 24, 2021

jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022

Merge pull request nasa#990 from jphickey/fix-988-printf-warnings

df2de6c

Fix nasa#988, add casts on printf calls

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #982, Add test for object id inline functions #990

Fix #982, Add test for object id inline functions #990

zanzaben commented May 7, 2021

jphickey left a comment

skliper commented May 12, 2021

jphickey commented May 12, 2021

skliper commented May 12, 2021 •

edited

Loading

astrogeco commented May 12, 2021 •

edited

Loading

astrogeco commented May 12, 2021

zanzaben commented May 13, 2021

jphickey left a comment

jphickey May 13, 2021

zanzaben May 14, 2021

jphickey commented May 14, 2021

jphickey commented May 14, 2021

astrogeco commented May 18, 2021

jphickey commented May 18, 2021

Fix #982, Add test for object id inline functions #990

Fix #982, Add test for object id inline functions #990

Conversation

zanzaben commented May 7, 2021

jphickey left a comment

Choose a reason for hiding this comment

skliper commented May 12, 2021

jphickey commented May 12, 2021

skliper commented May 12, 2021 • edited Loading

astrogeco commented May 12, 2021 • edited Loading

astrogeco commented May 12, 2021

zanzaben commented May 13, 2021

jphickey left a comment

Choose a reason for hiding this comment

jphickey May 13, 2021

Choose a reason for hiding this comment

zanzaben May 14, 2021

Choose a reason for hiding this comment

jphickey commented May 14, 2021

jphickey commented May 14, 2021

astrogeco commented May 18, 2021

jphickey commented May 18, 2021

skliper commented May 12, 2021 •

edited

Loading

astrogeco commented May 12, 2021 •

edited

Loading