[Test] cover the create() failure path of custom connection - #254
[Test] cover the create() failure path of custom connection#254myungjoo wants to merge 1 commit into
Conversation
SummaryPR #254 fully addresses issue #253's suggested scope: it adds failure injection to Verification performed
Blocking issuesNone found. Non-blocking nits
ConclusionApprove. The change is well-scoped, accurately verified against the real error path it targets (including a documented before/after check that reverting the #250 |
76b8e1c to
b2182a0
Compare
|
Addressed nit 1 in b2182a0: Nit 2 needs no change — the PR description already states that No other change; re-running the review on the updated commit. |
ScopeRe-review of PR #254 at the current head, commit Verification of the prior nitNit 1 ("the global test-control state is not synchronized — worth a one-line comment") is resolved.
This is accurate and sufficient — Independent verification performed
Blocking issuesNone. Non-blocking nits
ConclusionApprove. This re-review at |
|
Addressed the new nit: |
b2182a0 to
f6788ee
Compare
ScopeRe-review of PR #254 at the current head, commit Delta since the last review (
|
nns_edge_custom_load() reaches its error path in two ways: the library cannot be loaded, or the loaded library's nns_edge_custom_create() returns an error. Only the first was covered, by edgeCustom.createHandleLoadFail_n added for issue nnstreamer#249, even though both are governed by the same "*edge_h must not be left dangling" contract. The second path could not be reached from a test because the custom connection library for test only fails on !priv or OOM. Add a control block to it, exported as nns_edge_custom_test_ctrl and resolved by the test with dlsym() on the very library that nns_edge_custom_load() opens by name, so both refer to one instance: - create_error makes nns_edge_custom_create() fail on demand. - close_count and close_had_priv record how nns_edge_custom_close() was called, which pins down what a custom library gets after its own create() has failed. Every member is zero until a test sets it, so the library keeps its former behavior for every other user, and the test fixture clears the block in TearDown() so an injected failure cannot leak into another test. Add three tests on top of that: - createHandleCreateFail_n asserts nns_edge_custom_create_handle() returns the error the custom library raised and leaves *edge_h NULL. Reverting the issue nnstreamer#249 fix makes it fail. - loadCreateFail_n asserts nns_edge_custom_load() propagates the same error and leaves its own out-param untouched. - createHandleAfterCreateFail asserts a failed creation leaves nothing behind, so the hooks stay inert once the injection is cleared. Document the create()/close() contract that these tests pin down in nnstreamer-edge-custom.h: close() is invoked even when create() fails, and it then receives whatever create() left in priv. Link ${CMAKE_DL_LIBS} into the custom unittest, which now calls dlopen() itself. Fixes nnstreamer#253 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
|
Addressed the
|
f6788ee to
fda8f91
Compare
ScopeRe-review of PR #254 at the current head, commit Delta since the last review (
|
myungjoo-bot
left a comment
myungjoo-bot
left a comment
There was a problem hiding this comment.
Note: this review was produced by an automated review agent and is transcribed here by the bot account.
이 리뷰는 자동 코드 리뷰 에이전트(
github-code-review스킬)가 작성한 결과를 봇 계정이 옮겨 적은 것입니다.
fda8f91c 기준 리뷰입니다. 코드 자체에는 막을 만한 문제가 없습니다. 다만 아래 1번(#255와의 중복·충돌) 때문에 approve 대신 comment로 남깁니다 — 어느 쪽을 먼저 태울지는 메인테이너 판단이 필요합니다.
리뷰 범위
include/nnstreamer-edge-custom.h (+11, 주석만), tests/CMakeLists.txt (+1/-1), tests/nnstreamer-edge-custom-test.c (+9), tests/nnstreamer-edge-custom-test.h (신규 42줄), tests/unittest_nnstreamer-edge-custom.cc (+103).
확인한 것
- 주제 해결:
nns_edge_custom_load()의error:라벨에 이르는 두 경로 중create()실패 쪽이 미커버라는 진단이 맞고, 테스트용 커스텀 라이브러리에 실패 주입 스위치를 넣지 않고서는 공개 API로 그 경로를 밟을 수 없다는 설명도 코드상 정확합니다(기존nns_edge_custom_create()는!priv와 OOM에서만 실패). - 제어 블록 공유가 실제로 성립합니다: fixture가
dlopen("libnnstreamer-edge-custom-test.so")로 잡은 인스턴스와nns_edge_custom_load()가 같은 soname으로 여는 인스턴스는 동일 매핑이므로dlsym()으로 얻은 포인터가 유효합니다. 게다가 fixture가 테스트 내내 참조를 하나 붙들고 있어, 실패 경로에서_load_custom_library()가dlclose()를 해도 제어 블록이 언매핑되지 않습니다. 주석에 적힌 의도대로 동작합니다. - 격리:
SetUp()/TearDown()양쪽에서 제어 블록을 0으로 지우므로 주입된 실패가 다른 테스트로 새지 않습니다.SetUp()의 fatal 단언으로 본문이 스킵되어도TearDown()이 NULL 가드와 함께 돌아 안전합니다. 환경변수 방식보다 구조적으로 견고합니다. - 단언의 정확성:
createHandleCreateFail_n이 기대하는close_count == 1,close_had_priv == 0은 현재 구현과 일치합니다 —create()가 할당 없이 실패하므로custom->priv가 NULL인 채nns_edge_custom_release()가close(NULL)을 한 번 호출합니다.*edge_h == NULL단언은 #250이 넣은 라인을 되돌리면 실제로 깨진다는 저자 검증도 타당합니다(그 라인이 유일한 대입 지점입니다). - 회귀 위험:
src/무변경, 공개 헤더는 주석만(멤버 순서·타입·레이아웃 그대로라 ABI 영향 없음), 제어 블록은 zero-initialized라 스위치를 켜지 않는 기존 사용자에게 동작 변화가 없습니다.${CMAKE_DL_LIBS}추가는 테스트가 직접dlopen/dlsym을 쓰게 된 데 대한 정확한 대응이며 glibc에서는 빈 문자열로 전개됩니다. - CI: 새 테스트는 ubuntu cmake의
ctest(#251에서 등록)와 GBS의run_unittests.sh ./tests/unittest_nnstreamer-edge-custom양쪽에서 돌아 회귀를 막을 수 있습니다. 전 항목 green이고, 새 헤더도 doxygen/indent/newline 등 static check를 통과했습니다. - 보안: 백도어로 볼 만한 코드 없음. 실패 주입 심볼은 설치되지 않는 테스트 전용 헤더로 선언되고, 노출되는 전역은 unittest 서브패키지에만 들어가는 테스트 라이브러리에 있습니다.
지적 사항
- #255와 중복이고 텍스트 충돌합니다 (blocking). #255가 #252(릭)와 #253(이 PR이 닫으려는 이슈)을 함께 닫으면서,
create()실패 경로 테스트를 환경변수 스위치 방식으로 이미 넣고 있습니다. 두 head를 로컬에서 merge해 본 결과tests/nnstreamer-edge-custom-test.c와tests/unittest_nnstreamer-edge-custom.cc가 CONFLICT입니다(공개 헤더의 계약 주석도 양쪽이 각각 추가). 어느 쪽이 먼저 들어가든 나머지 하나는 손봐야 합니다.
권하는 순서는 릭 수정이 들어 있는 #255를 먼저 머지하고, 이 PR은 #255가 갖지 못한 부분만 남겨 리베이스하는 것입니다:- fixture + 제어 블록 방식(환경변수 전역 상태를 쓰지 않고,
TearDown()이 항상 정리) close_count/close_had_priv로 "create() 실패 후 close()가 어떻게 불렸는지"를 관측하는 부분 — 두 PR이 문서로만 적어 둔 계약을 실제로 고정하는 것은 이 PR뿐입니다.
반대로 이 PR을 먼저 태우기로 한다면, #255에서 중복되는createHandleCreateFail_n/loadCreateFail_n과 환경변수 스위치를 걷어내는 편이 깔끔합니다.
- fixture + 제어 블록 방식(환경변수 전역 상태를 쓰지 않고,
- (사소) PR 본문의 표에는
close_count/close_priv로 적혀 있는데 실제 멤버는close_had_priv입니다. 코드가 아니라 설명 쪽 오타입니다. - (사소)
create_error가 켜진 상태의nns_edge_custom_create()는!priv검증보다 먼저 반환합니다. 이 파일은 공개 헤더가 예제로 가리키는 참조 구현이니, 주입 검사를if (!priv)뒤로 내려 파라미터 검증 순서를 보존하는 편이 낫습니다. (#255의 환경변수 스위치도 같은 위치에 있어 동일한 지적을 남겼습니다.)
테스트 설계와 구현 자체는 견고합니다. 1번의 중복만 정리되면 approve할 수 있습니다.
2 participants
Resolves #253.
The gap
nns_edge_custom_load()reaches itserror:label in two ways:_load_custom_library()fails — covered byedgeCustom.createHandleLoadFail_n, added in [Fix] clear out-param when custom lib loading fails #250.nns_edge_custom_create()returns an error — not covered by anything.Both are subject to the same "
*edge_hmust not be left dangling" contract that #250 fixed and documented, so the second path deserves the same regression guard.Why it could not be tested
tests/nnstreamer-edge-custom-test.conly fails innns_edge_custom_create()on!privor OOM, neither of which a test can trigger through the public API. Path (2) therefore needed a failure-injection switch in the custom connection library for test.What this adds
A control block in the custom connection library for test, exported as
nns_edge_custom_test_ctrland declared in the newtests/nnstreamer-edge-custom-test.h. The unit test resolves it withdlsym()on the very library thatnns_edge_custom_load()opens by name, so both refer to one instance regardless of how the loader arranged things:create_error— makesnns_edge_custom_create()return that error without allocating.close_count/close_priv— record hownns_edge_custom_close()was called.Every member is zero until a test sets it, so the library behaves exactly as before for every other user, and the fixture clears the block in
TearDown()so an injected failure cannot leak into another test.Three tests on top of it:
edgeCustomFail.createHandleCreateFail_nnns_edge_custom_create_handle()returns the error the custom library raised, and leaves*edge_h == NULLedgeCustomFail.loadCreateFail_nnns_edge_custom_load()propagates the same error and leaves its own out-param untouchededgeCustomFail.createHandleAfterCreateFailVerified that the first one is a real guard: temporarily reverting the
*edge_h = NULL;line from #250 makes it fail withValue of: edge_h == __null / Actual: false, and restoring it makes it pass.The close()-after-failed-create() contract
The issue asked to settle this. On path (2)
custom->instanceis valid, sonns_edge_custom_release()proceeds and callscustom_h->nns_edge_custom_close (custom->priv)on aprivwhosecreate()just failed. Current behavior is kept — it gives a custom library the chance to release whatever it had allocated before failing — and is now documented on the twonns_edge_custom_smembers ininclude/nnstreamer-edge-custom.h, withclose_count/close_privpinning it down in the tests.No production logic is changed. Restructuring that error path belongs to #252, which rewrites the same
error:block.Scope and impact
include/nnstreamer-edge-custom.h— comment only; struct layout, member order and types are untouched, so no API or ABI change.tests/nnstreamer-edge-custom-test.c— shipped only in theunittestsub-package; the new global is zero-initialized, so its behavior is unchanged for existing users.tests/CMakeLists.txt—${CMAKE_DL_LIBS}on the custom unittest, which now callsdlopen()itself. It expands to nothing wheredlis already in libc.tests/nnstreamer-edge-custom-test.h— test-only, not installed; header installs are listed file by file insrc/CMakeLists.txt, and the debian build never setsENABLE_TEST.src/— untouched.CI coverage
The new tests run in both existing paths, so a regression cannot merge:
ctestin the ubuntu cmake workflow (registered viaADD_TESTwith theLD_LIBRARY_PATHproperty from #251) andrun_unittests.sh ./tests/unittest_nnstreamer-edge-customin the GBS build.Locally verified on Ubuntu 22.04 / gcc 11 / gtest 1.12.1:
unittest_nnstreamer-edge-custom29/29 andunittest_nnstreamer-edge171/171 pass,ctestreports 2/2, the build is free of warnings under-Wall -Werror,clang-formatreports no diff, andflawfinder --minlevel=1reports no hits on either changed C/C++ file.🤖 Generated with Claude Code