Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HOTFIX - no longer add unit tests from within unit tests in msg UT #840

Merged

Conversation

skliper
Copy link
Contributor

@skliper skliper commented Aug 25, 2020

Describe the contribution
HOTFIX - unit tests added from within unit tests will not execute, replaced this pattern with direct calls to the main subtest setup routine.

Testing performed
Build unit tests and ran, all tests (including subtests) ran.

Expected behavior changes
All tests run

System(s) tested on

  • Hardware: cFS Dev Server
  • OS: Ubuntu 18.04
  • Versions: integration candidate bundle + this commit

Additional context
None

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman - NASA/GSFC

@skliper skliper changed the base branch from main to integration-candidate August 25, 2020 17:30
@astrogeco
Copy link
Contributor

Does this mean that we need to revisit this behavior in UT_ADD_TEST()?

@yammajamma yammajamma merged commit 61a15e4 into nasa:integration-candidate Aug 25, 2020
@skliper
Copy link
Contributor Author

skliper commented Aug 25, 2020

I just closed the issue in OSAL as "wontfix" with the recommendation to avoid the pattern of adding tests from within other tests. I'd think this is enough (just don't do it). It is a result of developing the related tests on an older OSAL, then being surprised they no longer run. Could update things in ut_assert such that this could work (or at least reject the request), but not sure if it's worth the effort.

@astrogeco
Copy link
Contributor

Can you link that osal issue?

@astrogeco
Copy link
Contributor

I just closed the issue in OSAL as "wontfix" with the recommendation to avoid the pattern of adding tests from within other tests. I'd think this is enough (just don't do it). It is a result of developing the related tests on an older OSAL, then being surprised they no longer run. Could update things in ut_assert such that this could work (or at least reject the request), but not sure if it's worth the effort.

Just looking at the code we might want to add some comments for why some of these don't follow the semantic pattern. Since I can't tell from a quick glance why some of these use UT_ADD_TEST and others don't

    UT_ADD_TEST(Test_MSG_Init);
    UT_ADD_TEST(Test_MSG_CCSDSPri);
    Test_MSG_CCSDSPri();
    UT_ADD_TEST(Test_MSG_CCSDSExt);
    Test_MSG_CCSDSExt();
    UT_ADD_TEST(Test_MSG_MsgId_Shared);
    Test_MSG_MsgId_Shared();
    UT_ADD_TEST(Test_MSG_MsgId);
    UT_ADD_TEST(Test_MSG_MsgId);

@skliper
Copy link
Contributor Author

skliper commented Aug 25, 2020

Related to nasa/osal#577. Individual tests are added w/ the UT_ADD_TEST call. Some tests are grouped at a lower level (Test_MSG_CCSDSExt is a group of tests), and those functions are called directly. Same pattern is used in sb:

Test_SendMsg_API();
UtTest_Add(Test_RcvMsg_API, NULL, Test_CleanupApp_API, "Test_RcvMsg_API");

@astrogeco
Copy link
Contributor

astrogeco commented Aug 25, 2020

I just closed the issue in OSAL as "wontfix" with the recommendation to avoid the pattern of adding tests from within other tests. I'd think this is enough (just don't do it). It is a result of developing the related tests on an older OSAL, then being surprised they no longer run. Could update things in ut_assert such that this could work (or at least reject the request), but not sure if it's worth the effort.

I opened #841 to have a targeted discussion about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Msg module unit tests add tests within tests, which don't get executed with the current osal/ut_assert
3 participants