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

Clean up commented-out functions in unit test #102

Closed
jphickey opened this issue Dec 8, 2021 · 6 comments · Fixed by #328
Closed

Clean up commented-out functions in unit test #102

jphickey opened this issue Dec 8, 2021 · 6 comments · Fixed by #328
Labels
draco-rc4 unit-test Related to coverage or functional tests
Milestone

Comments

@jphickey
Copy link
Contributor

jphickey commented Dec 8, 2021

The unit test contains a number of test sequences that are completely disabled/commented out.

For example:

CF/unit-test/cf_cfdp_r_tests.c

Lines 1841 to 1876 in 7b99b91

void Test_CF_CFDP_R_SubstateSendNak_AssertsBecauseGiven_t_md_recv_Is_0_SendEvent_CallTo_CF_CFDP_SendNak_CannotReturn_CF_SEND_ERROR_ButDoesAnyway(
void)
{
// /* Arrange */
// history_t dummy_history;
// transaction_t dummy_t;
// transaction_t* arg_t = &dummy_t;
// pdu_nak_t dummy_nak;
// pdu_header_t* dummy_ph = (pdu_header_t*)&dummy_nak;
// CF_CFDP_ConstructPduHeader_context_t context_CF_CFDP_ConstructPduHeader;
// CF_CFDP_SendNak_context_t context_CF_CFDP_SendNak;
// int local_result;
// arg_t->history = &dummy_history;
// arg_t->flags.rx.md_recv = 0;
// context_CF_CFDP_ConstructPduHeader.forced_return = dummy_ph;
// UT_SetDataBuffer(UT_KEY(CF_CFDP_ConstructPduHeader), &context_CF_CFDP_ConstructPduHeader,
// sizeof(context_CF_CFDP_ConstructPduHeader), false);
// context_CF_CFDP_SendNak.forced_return = CF_SEND_ERROR;
// UT_SetDataBuffer(UT_KEY(CF_CFDP_SendNak), &context_CF_CFDP_SendNak,
// sizeof(context_CF_CFDP_SendNak), false);
// /* Act */
// local_result = CF_CFDP_R_SubstateSendNak(arg_t);
// /* Assert */
UtAssert_MIR("JIRA: GSFCCFS-1733 CF_Assert - sret!=CF_SEND_ERROR");
// UtAssert_True(local_result == -1,
// "CF_CFDP_R_SubstateSendNak returned %d and should be -1",
// local_result);
// UtAssert_STUB_COUNT(CF_CFDP_ConstructPduHeader, 1);
// UtAssert_STUB_COUNT(CFE_EVS_SendEvent, 1);
// UtAssert_STUB_COUNT(CF_CFDP_SendNak, 1);
} /* end

These serve no value, they are not testing anything, and only serve to clutter the code (it is not likely to work if un-commented, as it would be un-commented already if it did work).

Without a clear reason why these exist in the code, recommendation is to remove. Version control serves the purpose of preserving historical code, if the concern is to keep a historical record of a previous test case (it does not need to stay in source file).

@jphickey jphickey added the unit-test Related to coverage or functional tests label Dec 8, 2021
@skliper
Copy link
Contributor

skliper commented Mar 24, 2022

a7f89be was never merged, reopening since there's still many commented out sections in CF unit tests.

@thnkslprpt
Copy link
Contributor

a7f89be was never merged, reopening since there's still many commented out sections in CF unit tests.

Wasn't this all cleared up in 3acc447 Jake?

@skliper
Copy link
Contributor

skliper commented Oct 3, 2022

Looks like there's still a few stray lines of commented out code that need to get resolved. Note our coding standard discourages using the C++ style comments to begin with (no //). Might be good to clean that up at the same time.

grep -r " //" *
cf_app_tests.c:    // all values for dummy_table.ticks_per_second nominal except 0
cf_app_tests.c:    // all values (except 0) & 3ff == 0 are nominal (1024 byte aligned)
cf_app_tests.c:    // all values less than sizeof(CF_CFDP_PduFileDataContent_t) are nominal
cf_app_tests.c:    // outgoing_file_chunk_size set to greater than sizeof(CF_CFDP_PduFileDataContent_t)
cf_app_tests.c:    // UtAssert_STUB_COUNT(CF_TableInit, 1);
cf_cmd_tests.c:        if (catastrophe_count == 10) // 10 is arbitrary
cf_cmd_tests.c:    // CF_AppData.config_table = &dummy_config_table;
cf_cmd_tests.c:    //  memcpy(CF_AppData.config_table->chan[arg_chan_num].sem_name, dummy_sem_name, 20);
cf_utils_tests.c:    // /* Assert */
cf_utils_tests.c:    uint32 arg_read_size = Any_uint32_LessThan_or_EqualTo(10); // 10 is arbitrary to make test fast
cf_utils_tests.c:    uint8  dummy_buf[10] = {0};                                // 10 to match max read size of 10 (arbitrary)
utilities/cf_test_utils.c:    uint8 diff = ceiling - floor + 1; // +1 for inclusive

@skliper
Copy link
Contributor

skliper commented Oct 3, 2022

Really cf_app_tests.c, cf_cmd_tests.c, cf_timer_tests.c, and cf_utils_tests.c need the scrub all the other test code got. Removing random numbers, using modern API's, etc. That's a bigger job, but would finish the other tickets (#264, #114, #105, #87, #86).

@thnkslprpt
Copy link
Contributor

thnkslprpt commented Oct 4, 2022

@skliper so I think 656b2c3 is enough to address this issue in a narrow sense (and close it), and if there are already issues open for the more comprehensive clean-up, the rest can be addressed there.
Would you agree?

@skliper
Copy link
Contributor

skliper commented Oct 4, 2022

@thnkslprpt Yes, looks good!

dzbaker added a commit that referenced this issue Oct 18, 2022
Fix #102, Clean up remaining commented-out code
@dmknutsen dmknutsen added this to the Draco milestone Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
draco-rc4 unit-test Related to coverage or functional tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants