search

scitokens / scitokens-cpp

A C++ implementation of the SciTokens library with a C library interface
Apache License 2.0
5 stars 22 forks source link

Pr 113 metadata err msg #123

Closed jhiemstrawisc closed 1 year ago

jhiemstrawisc commented 1 year ago

This one fell through the cracks for me, but I'm taking over PR #113 to handle failed CI test and linter issues.

jhiemstrawisc commented 1 year ago

@bbockelm @djw8605 Okay, linter is happy and all tests are passing. One thing to point out is that I've noticed sporadic unit test failures with the Ubuntu build tests. I haven't been able to diagnose, but it usually seems like re-running the tests fixes things (as was the case with this PR -- I only formatted stuff from the linter). One issue with diagnosing is that we're not consistent about doing ASSERT_TRUE( rv == 0) << err_msg; to actually display the error. Should I change that in another PR?

In fact, if we do choose to display errors this way, I think we'll have to modify/override gtest's teardown function. When we call strdup in scitokens.cpp to populate err_msg, malloc is called under the hood; we should really be making sure that err_msg is freed any time an error is generated. I tested and confirmed with Valgrind that gtest does not free err_msg, resulting in leaks. Moreover, wherever we do ASSERT_FALSE( rv == 0 ), we should also be freeing the error message that we don't check.

Unfortunately, gtest skips the rest of the code block in a test once failure is encountered, so we can't free right after the assertion wherever an error message should correspond with a test failure. Hence the modification to gtest's teardown. Is this worth me tackling in another set of PRs, or do we not care since the unit tests are built/run in throwaway containers?

djw8605 commented 1 year ago

Could we do something as simple as just assigning the err_msg to a shared/unique ptr?