Add changes to enable audio playback test along with its dependencies#6
Conversation
vnarapar
force-pushed
the
audio_playback
branch
2 times, most recently
from
125ef93 to
2cf84bc
Compare
mwasilew
left a comment
mwasilew
left a comment
There was a problem hiding this comment.
You didn't add any tests for the new code and the coverage dropped below the threshold. Please fix that.
I think in the long term this change won't be needed as we'll be able to use s3:// URLs in LAVA. So I'm not sure it's worth adding it at all. I'm estimating s3 support should land in January.
As discussed we are not adding any additional coverage here as it will be validated as part of workflow |
I think you're missing the point. The patch adds new code to lava-test-plans. This code needs it's unit test. Please add it as a part of PR. Without unit tests this PR fails a code coverage check and won't be merged. |
vnarapar
force-pushed
the
audio_playback
branch
7 times, most recently
from
a072b3e to
9ae64e9
Compare
Added changes to generate a pre-signed URL and download the AudioClips from AWS when AUDIO_CLIPS_BASE_DIR is defined Updated formatting for lava_test_plans/__init__.py Signed-off-by: Vamsee Narapareddi <vnarapar@qti.qualcomm.com>
vnarapar
force-pushed
the
audio_playback
branch
from
9ae64e9 to
614e503
Compare
|
Thank you. This is great! Is there a chance you have an example job with audio overlay? I'll merge it now, but we need to make sure audio tests work before rolling out to meta-qcom |
mwasilew
left a comment
mwasilew
left a comment
There was a problem hiding this comment.
There is one minor and one major issue. The big problem is that the tests are added to the plan unconditionally. So they will be added even if the audio clips URL is not there. This will make (I didn't check that) the tests fail which will prevent any PRs in meta-qcom to be merged.
I'm OK to live with hardcored file name for now until we find a better solution
| name: "BT_ON_OFF" | ||
| path: Runner/suites/Connectivity/Bluetooth/BT_ON_OFF/BT_ON_OFF.yaml | ||
| repository: {{ TEST_DEFINITIONS_REPOSITORY }} | ||
| - from: git |
There was a problem hiding this comment.
This will be added even if the audio clips are not present. I think this isn't correct and you should add a conditional statement here.
There was a problem hiding this comment.
Yeah thats right, i have updated the check for AudioPlayback
| {% if AUDIO_CLIPS_URL is defined %} | ||
| - mkdir overlay_root | ||
| - tar -xzvf overlay.tar.gz -C overlay_root | ||
| - tar -xzvf AudioClips.tar.gz -C overlay_root |
There was a problem hiding this comment.
You're hardcoding file name. I know there is no way to use a variable in postprocess step, but maybe this should be a variable in a template with default value AudioClips.tar.gz.
vnarapar
force-pushed
the
audio_playback
branch
2 times, most recently
from
a9a743d to
88b9558
Compare
Below is the working Job: https://lava.infra.foundries.io/scheduler/job/134758 Looks like due to the PR being merged for Audio on qcom-linux-testkit i.e qualcomm-linux/qcom-linux-testkit#243 we are seeing issues with latest tip. Working with the PR owner to fix asap |
Added audio test dependent changes as an overlay Signed-off-by: Vamsee Narapareddi <vnarapar@qti.qualcomm.com>
Added Audio playback and Record testcases to premerge suite Signed-off-by: Vamsee Narapareddi <vnarapar@qti.qualcomm.com>
Enhanced the test coverage for the newly added code Signed-off-by: Vamsee Narapareddi <vnarapar@qti.qualcomm.com>
vnarapar
force-pushed
the
audio_playback
branch
from
88b9558 to
8f902cc
Compare
3 participants
from AWS when AUDIO_CLIPS_BASE_DIR is defined