Hi Ross,
Thank you for the detailed review and feedback!
I've addressed all your comments in v2:
1. Merged the duplicate test loops into a single loop
2. Removed || true from install commands
3. Hardcoded the list of helper binaries to install instead of dynamic detection
4. Simplified shell script installation using *.sh glob
5. Used ${TARGET_DBGSRC_DIR} instead of hardcoded /usr/src/debug path
6. Removed redundant RDEPENDS (bash and libcheck)
All 9 tests continue to pass on qemux86-64.
v2 patch is attached/sent separately.
Thanks again for the review!
Best Regards,
Pratik
________________________________________
From: Ross Burton <[email protected]>
Sent: Tuesday, November 18, 2025 1:10 PM
To: Pratik Farkase
Cc: [email protected]; [email protected]
Subject: Re: [OE-core] [PATCH] libcheck: add ptest support
Hi Pratik,
Thanks for adding a new test case. Some comments:
> On 14 Nov 2025, at 01:41, Pratik Farkase via lists.openembedded.org
> <[email protected]> wrote:
> +for test in check_check_export check_check; do
> + if [ -x ./$test ]; then
> + ./$test > /dev/null 2>&1
> + if [ $? -eq 0 ]; then
> + echo "PASS: $test"
> + else
> + echo "FAIL: $test"
> + fi
> + else
> + echo "SKIP: $test"
> + fi
> +done
> +
> +for test in test_output.sh test_check_nofork.sh
> test_check_nofork_teardown.sh \
> + test_xml_output.sh test_log_output.sh test_set_max_msg_size.sh \
> + test_tap_output.sh; do
> + if [ -x ./$test ]; then
> + ./$test > /dev/null 2>&1
> + if [ $? -eq 0 ]; then
> + echo "PASS: $test"
> + else
> + echo "FAIL: $test"
> + fi
> + else
> + echo "SKIP: $test"
> + fi
> +done
This looks like the same loop, twice. Why not merge them?
> + for binary in ${B}/tests/*; do
> + if [ -f "$binary" ] && [ -x "$binary" ]; then
> + case "$binary" in
> + *.sh) ;;
> + *) install -m 0755 "$binary" ${D}${PTEST_PATH}/tests/ ||
> true ;;
Don’t ||true, if it fails we want this to fail.
Also considering the lists of tests is hardcoded in run-ptest _and_ upstream
hasn’t been changed in four years, it might be easier to just hardcode the list
of binaries that we install instead of playing games trying to identify the
right output.
> + esac
> + fi
> + done
> +
> + for script in test_output.sh test_check_nofork.sh
> test_check_nofork_teardown.sh \
> + test_xml_output.sh test_log_output.sh
> test_set_max_msg_size.sh \
> + test_tap_output.sh; do
> + install -m 0755 ${S}/tests/$script ${D}${PTEST_PATH}/tests/
> + done
This is just test/*.sh, right?
> + install -m 0755 ${S}/tests/test_output_strings ${D}${PTEST_PATH}/tests/
> +
> + if [ -f ${B}/tests/test_vars ]; then
> + install -m 0644 ${B}/tests/test_vars ${D}${PTEST_PATH}/tests/
> + sed -i \
> + -e 's|if \[ x"[^"]*" != x"\." \];|if \[
> x"/usr/src/debug/libcheck/0.15.2/tests" != x"." \];|g' \
> + -e
> 's|SRCDIR="[^"]*"|SRCDIR="/usr/src/debug/libcheck/0.15.2/tests/"|g' \
> + ${D}${PTEST_PATH}/tests/test_vars
Hard-coded paths that will break on upgrade. ${TARGET_DBGSRC_DIR} is the
variable you want.
> +RDEPENDS:${PN}-ptest += "bash libcheck"
You can remove this: I don’t see any scripts that use bash, and the dependency
on libcheck will be generated automatically.
Thanks,
Ross
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#226527):
https://lists.openembedded.org/g/openembedded-core/message/226527
Mute This Topic: https://lists.openembedded.org/mt/116286168/21656
Group Owner: [email protected]
Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub
[[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
