-
Notifications
You must be signed in to change notification settings - Fork 27
Audio: Increase pre-merge test coverage to 10 test cases #243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| # Discover all audio clip files in the clips directory | ||
| # Returns: space-separated list of clip filenames (basenames only) | ||
| # Exit codes: 0=success, 1=directory not found or no clips | ||
| discover_audio_clips() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your header says it returns space-separated basenames, but it actually prints one basename per line.
That’s not inherently wrong, but many callers then do:
available_clips="$(discover_audio_clips 2>&1)" for clip in $available_clips; do ...
either update comment (“newline-separated list”), or output a single line.
Runner/utils/audio_common.sh
Outdated
| clip_filter="$2" | ||
|
|
||
| # Discover all available clips | ||
| available_clips="$(discover_audio_clips 2>&1)" || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either change discover_audio_clips to only print clips on stdout and log on stderr. Or don’t use 2>&1 here.
Runner/utils/audio_common.sh
Outdated
| # Count total clips first | ||
| idx=0 | ||
| for clip in $available_clips; do | ||
| idx=$((idx + 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arithmetic expansion $((...)) is not POSIX for all /bin/sh
Use expr or awk, or better, avoid manual counting and use set --
Runner/utils/audio_common.sh
Outdated
| clips_dir="${AUDIO_CLIPS_BASE_DIR:-AudioClips}" | ||
|
|
||
| # If name already looks like a filename, try direct path | ||
| if printf '%s' "$name" | grep -q '\.wav$'; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grep -F -q -- "$search_name"
Same issue exists in apply_clip_filter where grep -q "$pattern" treats pattern as regex. If you intend substring matching, use grep -F.
Runner/utils/audio_common.sh
Outdated
| # Extract components using sed | ||
| rate="$(printf '%s' "$filename" | sed -n 's/.*_\([0-9.]\+KHz\)_.*/\1/p')" | ||
| bits="$(printf '%s' "$filename" | sed -n 's/.*_\([0-9]\+b\)_.*/\1/p')" | ||
| channels="$(printf '%s' "$filename" | sed -n 's/.*_\([0-9]\+ch\)\.wav$/\1/p')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heavy sed for rate, bits and channels. Tighten patterns to match exact 4-field structure from end:
RATE..._BITS_CHANNELS.wav anchored near the end.
| clip_path="$clips_dir/$clip_file" | ||
|
|
||
| # Validate clip file | ||
| if ! validate_clip_file "$clip_path"; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make Config mapping explicit and stable (Config1 → specific descriptive testcase name / specific filename pattern), not “Nth entry”.
Or define a fixed list inside audio_common.sh for the 20 configs.
Otherwise, Config5 might test a different format on a different system/release, defeating CI intent.
| if [ "$rc" -eq 0 ]; then | ||
| log_pass "[$case_name] loop $i OK (rc=0, ${last_elapsed}s)" | ||
| ok_runs=$((ok_runs + 1)) | ||
| elif [ "$rc" -eq 124 ] && [ "$dur_s" -gt 0 ] 2>/dev/null && [ "$last_elapsed" -ge "$min_ok" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compute expected duration for the clip or remove timeout-pass heuristic for clip mode. If clips are 30s, you can set dur_s=30 and min_ok to a sane threshold.
Runner/utils/audio_common.sh
Outdated
| fi | ||
|
|
||
| # Check not empty | ||
| size="$(stat -c '%s' "$clip_path" 2>/dev/null || wc -c < "$clip_path" 2>/dev/null)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stat -c '%s' (GNU stat). Your fallback to wc -c < file helps, but you still call GNU stat first in many places. either use wc -c consistently (simple and portable)
or centralize “file_size_bytes()” helper with robust fallbacks.
| --formats "wav" | ||
| --durations "short|short medium|short medium long" | ||
| --formats "wav" # DEPRECATED: Use clip discovery instead | ||
| --durations "short|short medium" # DEPRECATED: Use clip discovery instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why comment says deprecated when these are being used?
|
|
||
| # Collect evidence once at end (not per clip) | ||
| if [ "$DMESG_SCAN" -eq 1 ]; then | ||
| scan_audio_dmesg "$LOGDIR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scans dmesg once per config(inside config loop). That can be heavy for 10 configs and makes attribution unclear. Scan once at the end of the suite (and optionally on first failure)
6fbded5 to
f691a38
Compare
PR Response - Review Comments AddressedHi @smuppand, Thanks for the thorough review. I've pushed an updated commit that addresses all the issues you raised. Changes MadePOSIX Compliance & Portability:
I/O Separation:
Config Stability:
Code Optimization:
CI/LAVA Support:
Performance:
Documentation:
Please review the latest changes and let me know if any further modifications are required. |
| log_info "Using config discovery mode" | ||
|
|
||
| # Discover and filter configs | ||
| CONFIGS_TO_TEST="$(discover_and_filter_record_configs "$CONFIG_NAMES" "$CONFIG_FILTER")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONFIGS_OUTPUT is undefined, so that branch can’t work as intended. Playback does this correctly, Do the same for record as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current AudioRecord code already uses the correct variable name CONFIGS_TO_TEST and follows the same pattern as AudioPlayback. There is no CONFIGS_OUTPUT variable in the current code.
| log_skip "$TESTNAME SKIP - $ERROR_MSG" | ||
| fi | ||
| echo "$TESTNAME SKIP" > "$RES_FILE" | ||
| exit 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use exit 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed all exit 2 to exit 0 for SKIP conditions in AudioRecord/run.sh to match framework standard (PASS=0, FAIL=1, SKIP=0).
| if [ "$USE_CLIP_DISCOVERY" = "auto" ]; then | ||
| # Auto mode: use clip discovery if AudioClips directory exists, otherwise legacy | ||
| clips_dir="${AUDIO_CLIPS_BASE_DIR:-AudioClips}" | ||
| if [ -d "$clips_dir" ] && [ -n "$(find "$clips_dir" -maxdepth 1 -name "*.wav" -type f 2>/dev/null | head -n1)" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-maxdepth is not posix, Replace with a simpler pattern or tolerate scanning a little deeper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced find -maxdepth with POSIX-compliant shell glob pattern in AudioPlayback/run.sh.
|
Hi @smuppand,
Please review the latest changes and let me know if any further modifications are required. |
|
@tmoida Please follow the guidelines. Reply to the corresponding review comment for which your latest changes resolved. It will be easy to review. Otherwise, it is hard to review and go through all files. |
| # ========== AudioPlayback Test Cases (7 configs) ========== | ||
|
|
||
| # Playback Test 1: Config1 (16KHz, 16-bit, 2ch) | ||
| - $PWD/suites/Multimedia/Audio/AudioPlayback/run.sh --clip-name "Config1" --res-suffix "Config1" --audio-clips-path /home/AudioClips/ --no-extract-assets || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In both scripts you do:
--clip-name/--config-name → sets USE_DISCOVERY=true
later --formats/--durations → currently sets USE_DISCOVERY=false
And your YAML currently passes --clip-name/--config-name before --formats/--durations, so the later legacy args can override discovery mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Moved default value assignment (FORMATS/DURATIONS) to after the conflict check. Now defaults are only set when in legacy mode, preventing false positive conflicts in discovery mode.
| if ! validate_clip_file "$clip_path"; then | ||
| log_skip "[$case_name] SKIP: Invalid clip file: $clip_path" | ||
| echo "$case_name SKIP (invalid file)" >> "$LOGDIR/summary.txt" | ||
| skip=$(expr $skip + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old bug in the code. Please fix it. Make LOGDIR absolute to the test directory.
If the script is invoked from Runner/ (as your premerge plan does), mkdir happens under Runner/results/..., but later file writes happen after cd and expect results/... under the suite directory. That can lead to missing directories and failed redirects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
LOGDIR now uses absolute path
| # Validate clip file | ||
| if ! validate_clip_file "$clip_path"; then | ||
| log_skip "[$case_name] SKIP: Invalid clip file: $clip_path" | ||
| echo "$case_name SKIP (invalid file)" >> "$LOGDIR/summary.txt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anchor .res paths to SCRIPT_DIR for early failures + keep exit-code behavior consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
RES_FILE now uses absolute path
Runner/utils/audio_common.sh
Outdated
| # Extract rate, bits, and channels in one sed call | ||
| # Pattern matches exact 4-field structure from end: _RATE_DURATIONs_BITS_CHANNELS.wav | ||
| # Anchored to .wav extension to ensure we're matching the correct fields | ||
| metadata="$(printf '%s' "$filename" | sed -n 's/.*_\([0-9.]\+KHz\)_\([0-9]\+s\)_\([0-9]\+b\)_\([0-9]\+ch\)\.wav$/\1 \3 \4/p')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the sed pattern contains literal ... ([0-9....Hz) which will never match real filenames.
Fix it to a real, portable pattern (also removes GNU +)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several sed patterns using + (GNU extension). Since the repo repeatedly demands POSIX/ShellCheck clean, this will bite across Yocto/busybox variants.
Replace things like:
([[:alnum:]]+) → ([[:alnum:]][[:alnum:]_])
[0-9]+ → [0-9][0-9]
[0-9.]+ → [0-9][0-9.]* (or [0-9.][0-9.]*)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. All sed patterns in audio_common.sh now use POSIX-compliant syntax:
- Changed
[0-9]\+to[0-9][0-9]*(one digit followed by zero or more) - Changed
[0-9.]\+to[0-9.][0-9.]*for decimal numbers - Changed
[[:alnum:]_]\+to[[:alnum:]_][[:alnum:]_]*
| RES_FILE="./${TESTNAME}.res" | ||
| LOGDIR="results/${TESTNAME}" | ||
| # Use absolute paths for LOGDIR to work from any directory | ||
| LOGDIR="$SCRIPT_DIR/results/${TESTNAME}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RES_SUFFIX is used, but logs will overwrite across multiple runs
You suffix the result file, but LOGDIR is still constant (logs_AudioPlayback, logs_AudioRecord).
In the premerge plan you run multiple configs back-to-back → logs collide/overwrite in both run.sh files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. LOGDIR now applies the same suffix as RES_FILE to create unique log directories per invocation:
Applied to both AudioPlayback and AudioRecord. The script logs "Using unique log directory: ..." when a suffix is provided.
This matches the RES_FILE behavior and prevents log collisions in CI/LAVA workflows with back-to-back runs.
| - ./run.sh --backend "${AUDIO_BACKEND}" --sink "${SINK_CHOICE}" --formats "${FORMATS}" --durations "${DURATIONS}" --loops "${LOOPS}" --timeout "${TIMEOUT}" --strict "${STRICT}" --audio-clips-path "${AUDIO_CLIPS_BASE_DIR}" --ssid "${SSID}" --password "${PASSWORD}" || true | ||
| - $REPO_PATH/Runner/utils/send-to-lava.sh AudioPlayback.res || true | ||
| - ./run.sh --backend "${AUDIO_BACKEND}" --sink "${SINK_CHOICE}" --clip-name "${CLIP_NAMES}" --clip-filter "${CLIP_FILTER}" --formats "${FORMATS}" --durations "${DURATIONS}" --loops "${LOOPS}" --timeout "${TIMEOUT}" --strict "${STRICT}" --audio-clips-path "${AUDIO_CLIPS_BASE_DIR}" --res-suffix "${RES_SUFFIX}" --ssid "${SSID}" --password "${PASSWORD}" || true | ||
| - $REPO_PATH/Runner/utils/send-to-lava.sh AudioPlayback.res || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But when RES_SUFFIX is set, your script writes:
AudioPlayback_Config1.res (etc), not AudioPlayback.res.
So LAVA will upload the wrong / stale / missing .res whenever suffix is used. update YAML to send ${TESTNAME}_${RES_SUFFIX}.res when RES_SUFFIX is set.
Do the same check for AudioRecord.yaml (it currently also sends AudioRecord.res unconditionally).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
YAML patterns now use POSIX parameter expansion to handle both suffixed and non-suffixed cases correctly. Applied to both AudioPlayback.yaml and AudioRecord.yaml.
| log_fail "Overlay audio environment setup failed" | ||
| echo "$TESTNAME FAIL" > "$RES_FILE" | ||
| # Use absolute path for result file (early failure before RES_FILE is set) | ||
| echo "$TESTNAME FAIL" > "$SCRIPT_DIR/${TESTNAME}.res" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You fixed suffixing after CLI parse, but both AudioPlayback/run.sh and AudioRecord/run.sh still do That happens before parsing --res-suffix. If overlay setup fails in parallel CI runs, you’re back to collisions on AudioPlayback.res / AudioRecord.res.
do a minimal “pre-parse” of just --res-suffix before calling setup_overlay_audio_environment, so even early failures write to the suffixed res.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Added pre-parsing of --res-suffix before setup_overlay_audio_environment() to ensure unique result files even on early failures. Applied to both AudioPlayback and AudioRecord.
| # Aggregate result for this clip | ||
| if [ "$ok_runs" -ge 1 ]; then | ||
| pass=$(expr $pass + 1) | ||
| echo "$case_name PASS" >> "$LOGDIR/summary.txt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
summary.txt is appended but not reset
You append per-test results
echo "$case_name PASS" >> "$LOGDIR/summary.txt"
But I didn’t see : > "$LOGDIR/summary.txt" after creating LOGDIR. That will accumulate results across repeated invocations in the same workspace (classic CI flake source).
truncate it once after mkdir -p "$LOGDIR".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Added truncation of summary.txt immediately after LOGDIR creation to prevent accumulation across repeated invocations. Applied to both AudioPlayback and AudioRecord.
This ensures each test run starts with a clean summary.txt file.
smuppand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nit changes.
| CONFIG_NAMES: "record_config1" # Test specific configs (e.g., "record_config1 record_config2"), default: record_config1 | ||
| CONFIG_FILTER: "" # Filter configs by pattern (e.g., "48KHz" or "2ch"), default: unset | ||
| DURATIONS: "short" # Recording durations: short, medium, long, default: short | ||
| RECORD_SECONDS: 5 # Number of seconds to record (numeric or mapped), default: 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RECORD_SECONDS default to "5s" (or ensure script robustly accepts plain 5 everywhere).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Changed the YAML default from 5 to "5s" to match the expected format. Now both the YAML and script use the same format with unit suffix.
| else | ||
| echo "$TESTNAME FAIL" > "$SCRIPT_DIR/${TESTNAME}.res" | ||
| fi | ||
| exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer always exit 0 after writing FAIL to .res
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Changed the early failure exit code from exit 1 to exit 0 in both AudioPlayback and AudioRecord scripts.
Expanded audio testing from 2 to 10 test cases in the pre-merge CI pipeline to improve validation coverage across different audio formats and configurations. Changes: - AudioRecord: Added support for 10 recording configurations - AudioPlayback: Added support for 20 playback configurations - meta-ar-ci-premerge.yaml: Updated to run 7 playback + 3 record tests - Added --res-suffix parameter for unique result files and log directories - Fixed LOGDIR collision across multiple test runs in CI/LAVA environments - Fixed summary.txt accumulation bug across repeated invocations - Fixed early failure handling to ensure unique result files in parallel CI - Fixed YAML to send correct suffixed result files to LAVA - Updated documentation with CI/LAVA workflow examples AudioRecord enhancements: * Implemented config discovery mode with 10 predefined test configurations * Added --config-name and --config-filter CLI options for flexible test selection * Added --res-suffix option to generate unique result files and log directories (e.g., AudioRecord_Config1.res and results/AudioRecord_Config1/) * Fixed LOGDIR to apply suffix, preventing log overwriting in back-to-back runs * Fixed summary.txt truncation after LOGDIR creation to prevent accumulation * Added pre-parsing of --res-suffix for early failure handling before CLI parsing * Updated documentation with configuration table and usage examples * Added CI/LAVA workflow examples with validated output * Modified YAML to support new config parameters (defaults to record_config1) * Updated YAML with RES_SUFFIX parameter for parallel test execution AudioPlayback enhancements: * Implemented clip discovery mode with 20 predefined test configurations * Added --clip-name and --clip-filter CLI options for flexible test selection * Added --res-suffix option to generate unique result files and log directories (e.g., AudioPlayback_Config1.res and results/AudioPlayback_Config1/) * Fixed LOGDIR to apply suffix, preventing log overwriting in back-to-back runs * Fixed summary.txt truncation after LOGDIR creation to prevent accumulation * Added pre-parsing of --res-suffix for early failure handling before CLI parsing * Updated documentation with configuration table and usage examples * Added CI/LAVA workflow examples with validated output * Modified YAML to support new clip parameters (defaults to Config1) * Updated YAML with RES_SUFFIX parameter for parallel test execution Pre-merge CI updates: * 7 playback tests: Config1, Config7, Config13, Config15, Config18, Config20, Config5 * 3 record tests: record_config1, record_config7, record_config10 * Uses pre-staged clips at /home/AudioClips/ for faster execution * 10-second recording duration for efficient CI runtime * Unique result files and log directories prevent overwriting in parallel execution * Each test run creates isolated logs (e.g., results/AudioPlayback_Config1/) * Early failures now write to correct suffixed result files * LAVA correctly uploads suffixed result files for each test Test coverage now includes: - Sample rates: 8KHz to 384KHz - Bit depths: 8-bit to 32-bit - Channel configs: Mono, Stereo Signed-off-by: Teja Swaroop Moida <tmoida@qti.qualcomm.com>
smuppand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Expanded audio testing from 2 to 10 test cases in the pre-merge CI pipeline to improve validation coverage across different audio formats and configurations.
Changes:
AudioRecord enhancements:
AudioPlayback enhancements:
Pre-merge CI updates:
Test coverage now includes: