Fix #102, clean up sc_loads_test

[jphickey]

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I signed and emailed the appropriate Contributor License Agreement to GSFC-SoftwareRelease@mail.nasa.gov and copied cfs-program@lists.nasa.gov.

Describe the contribution
Significant buffer management issues existed in many/all of these test cases and was causing stack corruption. Rewriting the test cases to better follow current recommended practices, and reduce repetition.

Fixes #102

Testing performed
Build and run sc_loads_test, confirm coverage is maintained

Expected behavior changes
No more stack overflow/smashing.

System(s) tested on
Debian

Additional context
Test case code reduced by about 40%, should be much easier to maintain going forward.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

[skliper]

@jphickey - I already fixed all of this with #98. Whatever this commit is on doesn't seem in sync w/ main.

See my comment: #102 (comment)

[skliper]

This looks like a backwards step from #98 since it reintroduces local tables instead of initializing them in setup.

[jphickey]

@jphickey - I already fixed all of this with #98. Whatever this commit is on doesn't seem in sync w/ main.

See my comment: #102 (comment)

Yep, I noticed that! You had already fixed the bulk of what I had noticed when trying to run this test - buffer overruns galore. I rebased my change.

My change is still worthwhile to consolidate a lot of the repeated logic and use more of the modern test macros - e.g. I also got rid of all remaining "UtAssert_True" checks in favor of the more targeted version.

[jphickey]

I see that you instantiated the UT tables in unit-test/utilities/sc_test_utils.c ... I will update this to use those tables rather than an instance in the loads test file.

[skliper]

Your change now deviates from the pattern I employed for the rest of the SC tests, with the tables defined and initialized in the setup. I hadn't fixed the UtAssert_Trues which would be a good fix but I'd suggest sticking with the pattern established in #98 for the tables. After that, I had also consolidated some logic to avoid repeats... really hard to tell based on how this commit was done what's better vs just different since your change wasn't based off main to begin with. I'm not sure a step backwards from consistency employed for the other tests is helpful.

[jphickey]

@skliper - not intending to take a "step backwards" -- I like the pattern there in #98 and will use it, its just that I was basing this off equuleus-rc1 tag that didn't have that rework yet (because this tag is what the stakeholder is using). Which is unfortunate - #98 (comment)

I will update this to use the shared table instances. That will cut down on repetition even more. I just wish we had merged that when you first put in the PR. Would have saved some duplicate work.

[jphickey]

@skliper -- What's left in here is basically consolidation of repeated code during init of the ATS/RTS tables. Should make the test cases easier to maintain going forward.

[skliper]

@jphickey - great to see the UtAssert_True's go away also! Maybe that API could be deprecated some day.

[dzbaker]

CCB 21 September 2023: Provisionally approved pending re-review by @skliper.

[skliper]

@dzbaker - review is pending response to my previous comment

[jphickey]

Updated based on @skliper review comment and rebased to resolve merge conflict. Should be good to go now.

Changes made

[skliper]

Great set of updates!