nasa / CF

The Core Flight System (cFS) CFDP application.
Apache License 2.0
79 stars 45 forks source link

Clean up commented-out functions in unit test #102

Closed jphickey closed 1 year ago

jphickey commented 2 years ago

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

For example: https://github.com/nasa/CF/blob/7b99b91cd50a347f8553fc68ea3b074ff0672251/unit-test/cf_cfdp_r_tests.c#L1841-L1876

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).

skliper commented 2 years ago

https://github.com/nasa/CF/commit/a7f89beac0019073eeaab56ec01ce5476292c7c3 was never merged, reopening since there's still many commented out sections in CF unit tests.

thnkslprpt commented 1 year ago

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

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

skliper commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@skliper so I think 656b2c3e21bc475582a36e6ec7e6aed1e1f4a376 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 commented 1 year ago

@thnkslprpt Yes, looks good!